Add isPlainObject util, and use it for v.record - also add v.pojo() schema#1429
Open
EskiMojo14 wants to merge 7 commits intoopen-circle:mainfrom
Open
Add isPlainObject util, and use it for v.record - also add v.pojo() schema#1429EskiMojo14 wants to merge 7 commits intoopen-circle:mainfrom
isPlainObject util, and use it for v.record - also add v.pojo() schema#1429EskiMojo14 wants to merge 7 commits intoopen-circle:mainfrom
Conversation
|
@EskiMojo14 is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
the
size:XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
library/src/utils/isPlainObject/isPlainObject.test.ts (1)
5-29: Consider adding an explicit cross-realm POJO test.The current suite is strong, but the PR explicitly claims cross-realm support. Adding one dedicated cross-realm case would lock that guarantee against regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/utils/isPlainObject/isPlainObject.test.ts` around lines 5 - 29, Add a dedicated cross-realm POJO test to the isPlainObject test suite: create an object from a different realm (e.g., using Node's vm to run '({})' in a new context or an iframe in browser runs) and assert isPlainObject returns true for that object; update library/src/utils/isPlainObject/isPlainObject.test.ts to include this case alongside the existing plain-object tests so cross-realm behavior of isPlainObject is explicitly covered.library/src/schemas/pojo/pojo.ts (1)
57-63: Missing blank line between interface definition and JSDoc.There should be a blank line between the closing brace of
PojoSchemainterface and the JSDoc comment for thepojofunction, for consistency with the rest of the codebase.Suggested fix
readonly message: TMessage; } + /** * Creates a POJO schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/schemas/pojo/pojo.ts` around lines 57 - 63, The JSDoc for the pojo() function is directly adjacent to the closing brace of the PojoSchema interface; add a single blank line between the end of the PojoSchema interface (the closing "}") and the JSDoc block above the export function pojo(): PojoSchema<undefined> so the file matches the project's spacing conventions and other schema declarations.library/src/schemas/pojo/pojo.test.ts (1)
36-46: Consider adding a test for cross-realm objects.The PR objectives mention that the
isPlainObjectimplementation handles cross-realm POJOs. Consider adding a test to verify this behavior, though this may be better suited for theisPlainObjectutility tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/schemas/pojo/pojo.test.ts` around lines 36 - 46, Add a test in pojo.test.ts that verifies cross-realm plain objects are accepted: create an object from another realm (e.g., using Node's vm.Module/VM or an iframe in browser tests) and call expectNoSchemaIssue(schema, [crossRealmObj]) where schema = pojo(); reference the existing test harness (expectNoSchemaIssue and pojo()) and ensure the new test mirrors the "for null prototype" style so it asserts the schema accepts cross-realm POJOs handled by isPlainObject.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/schemas/record/recordAsync.ts`:
- Line 138: The async branch in recordAsync (the guard using
isPlainObject(input) in recordAsync.ts) lacks test coverage because the
async-array rejection test in recordAsync.test.ts is disabled; either re-enable
and fix that test (lines ~117–121) to assert that passing an array to the async
record function rejects (mirror the sync behavior) or explicitly document in
recordAsync.ts why async differs from record if you intend divergence—locate the
test referencing the async record function and update it to await the call and
expect a rejection for arrays, or add a clear TODO/comment near the
isPlainObject(input) guard explaining the intentional difference and linking to
the test decision.
---
Nitpick comments:
In `@library/src/schemas/pojo/pojo.test.ts`:
- Around line 36-46: Add a test in pojo.test.ts that verifies cross-realm plain
objects are accepted: create an object from another realm (e.g., using Node's
vm.Module/VM or an iframe in browser tests) and call expectNoSchemaIssue(schema,
[crossRealmObj]) where schema = pojo(); reference the existing test harness
(expectNoSchemaIssue and pojo()) and ensure the new test mirrors the "for null
prototype" style so it asserts the schema accepts cross-realm POJOs handled by
isPlainObject.
In `@library/src/schemas/pojo/pojo.ts`:
- Around line 57-63: The JSDoc for the pojo() function is directly adjacent to
the closing brace of the PojoSchema interface; add a single blank line between
the end of the PojoSchema interface (the closing "}") and the JSDoc block above
the export function pojo(): PojoSchema<undefined> so the file matches the
project's spacing conventions and other schema declarations.
In `@library/src/utils/isPlainObject/isPlainObject.test.ts`:
- Around line 5-29: Add a dedicated cross-realm POJO test to the isPlainObject
test suite: create an object from a different realm (e.g., using Node's vm to
run '({})' in a new context or an iframe in browser runs) and assert
isPlainObject returns true for that object; update
library/src/utils/isPlainObject/isPlainObject.test.ts to include this case
alongside the existing plain-object tests so cross-realm behavior of
isPlainObject is explicitly covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 670a4389-0857-4eb2-bca1-b9ee3768b9eb
📒 Files selected for processing (26)
library/src/schemas/index.tslibrary/src/schemas/pojo/index.tslibrary/src/schemas/pojo/pojo.test-d.tslibrary/src/schemas/pojo/pojo.test.tslibrary/src/schemas/pojo/pojo.tslibrary/src/schemas/record/record.test.tslibrary/src/schemas/record/record.tslibrary/src/schemas/record/recordAsync.tslibrary/src/utils/index.tslibrary/src/utils/isPlainObject/index.tslibrary/src/utils/isPlainObject/isPlainObject.test.tslibrary/src/utils/isPlainObject/isPlainObject.tswebsite/src/routes/api/(schemas)/looseObject/index.mdxwebsite/src/routes/api/(schemas)/object/index.mdxwebsite/src/routes/api/(schemas)/objectWithRest/index.mdxwebsite/src/routes/api/(schemas)/pojo/index.mdxwebsite/src/routes/api/(schemas)/pojo/properties.tswebsite/src/routes/api/(schemas)/record/index.mdxwebsite/src/routes/api/(schemas)/strictObject/index.mdxwebsite/src/routes/api/(types)/PojoIssue/index.mdxwebsite/src/routes/api/(types)/PojoIssue/properties.tswebsite/src/routes/api/(types)/PojoSchema/index.mdxwebsite/src/routes/api/(types)/PojoSchema/properties.tswebsite/src/routes/api/menu.mdwebsite/src/routes/guides/(main-concepts)/schemas/index.mdxwebsite/src/routes/guides/(schemas)/objects/index.mdx
Member
Author
|
undecided if we should use |
EskiMojo14
commented
Mar 17, 2026
|
|
||
| // If root type is valid, check nested types | ||
| if (input && typeof input === 'object') { | ||
| if (input && isPlainObject(input)) { |
Member
Author
There was a problem hiding this comment.
this could just be if (isPlainObject(input)) but i think a falsy check will probably fail faster
Member
|
Do you think there will be a meaningful performance impact? |
Member
Author
|
depends on how slow |
Member
Author
|
it would probably be quicker without the loop ( |
|
BTW you might have a look at this implem: https://github.com/belgattitude/httpx/tree/main/packages/plain-object#readme. 100% redux compat, might be 2x-4x times faster depending (see benchmarks). Tested on bun, node, chrome, workers. import type { BasePlainObject, DefaultBasePlainObject } from './internal.types';
import type { PlainObject } from './plain-object.types';
/**
* Check if a value is a plain object
*
* A plain object is a basic JavaScript object, such as {}, { data: [] }, new Object() or Object.create(null).
*
* @example
* ```typescript
* import { isPlainObject } from '@httpx/plain-object';
*
* // ✅👇 True
*
* isPlainObject({ }); // ✅
* isPlainObject({ key: 'value' }); // ✅
* isPlainObject({ key: new Date() }); // ✅
* isPlainObject(new Object()); // ✅
* isPlainObject(Object.create(null)); // ✅
* isPlainObject({ nested: { key: true} }); // ✅
* isPlainObject(new Proxy({}, {})); // ✅
* isPlainObject({ [Symbol('tag')]: 'A' }); // ✅
*
* // ✅👇 (node context, workers, ...)
* const runInNewContext = await import('node:vm').then(
* (mod) => mod.runInNewContext
* );
* isPlainObject(runInNewContext('({})')); // ✅
*
* // ❌👇 False
*
* class Test { };
* isPlainObject(new Test()) // ❌
* isPlainObject(10); // ❌
* isPlainObject(null); // ❌
* isPlainObject('hello'); // ❌
* isPlainObject([]); // ❌
* isPlainObject(new Date()); // ❌
* isPlainObject(new Uint8Array([1])); // ❌
* isPlainObject(Buffer.from('ABC')); // ❌
* isPlainObject(Promise.resolve({})); // ❌
* isPlainObject(Object.create({})); // ❌
* isPlainObject(new (class Cls {})); // ❌
*
* // ⚠️ Edge cases
* //
* // 👇 globalThis isn't properly portable across all JS environments
* //
*
* isPlainObject(globalThis); // ✅ with Bun ❌ otherwise (browser, Nodejs, edge, cloudflare)
*
* // 👇 Static built-in classes aren't properly checked. This is a trade-off
* // to maintain the best performance and size. If you need to check for these,
* // use a custom type guard. But in most cases, you won't need to check for these
* // as the probability of writing a code that receives these as plain objects is low.
* // and probably indicates an issue in your code.
*
* isPlainObject(Math); // ⚠️✅ return true, but Math is not a plain object
* isPlainObject(JSON); // ⚠️✅ return true, but JSON is not a plain object
* isPlainObject(Atomics); // ⚠️✅ return true, but Atomics is not a plain object
* isPlainObject(Reflect); // ⚠️✅ return true, but Reflect is not a plain object
* ```
*/
export const isPlainObject = <
TValue extends BasePlainObject = DefaultBasePlainObject,
>(
v: unknown
): v is TValue extends DefaultBasePlainObject
? BasePlainObject
: PlainObject<TValue> => {
if (v === null || typeof v !== 'object') {
return false;
}
const proto = Object.getPrototypeOf(v) as typeof Object.prototype | null;
return (
proto === null ||
proto === Object.prototype ||
// Required to support node:vm.runInNewContext({})
Object.getPrototypeOf(proto) === null
);
};Test file: |
Member
|
If we implement this, does parsing env variables still work? I remember that env variables (I think it was Node.js) were not a plain object. We should check that before we continue with this PR. |
fabian-hiller
added
the
next version
Member
Author
looks like you're right, process.env is not a plain object. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/src/routes/api/(schemas)/object/index.mdx (1)
36-36: Polish repetitive sentence openings in the explanation block.Line 36 starts multiple consecutive sentences with “To”, which reads a bit choppy. A small rewording would improve flow.
✍️ Suggested wording
-> This schema removes unknown entries. The output will only include the entries you specify. To include unknown entries, use <Link href="../looseObject/">`looseObject`</Link>. To return an issue for unknown entries, use <Link href="../strictObject/">`strictObject`</Link>. To include and validate unknown entries, use <Link href="../objectWithRest/">`objectWithRest`</Link>. If you only need to validate that the input is a plain object, use <Link href="../pojo/">`pojo`</Link>. +> This schema removes unknown entries, so the output only includes the entries you specify. Use <Link href="../looseObject/">`looseObject`</Link> to include unknown entries, <Link href="../strictObject/">`strictObject`</Link> to return an issue for unknown entries, and <Link href="../objectWithRest/">`objectWithRest`</Link> to include and validate unknown entries. If you only need plain-object root validation, use <Link href="../pojo/">`pojo`</Link>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/src/routes/api/`(schemas)/object/index.mdx at line 36, The explanation paragraph is choppy because multiple consecutive sentences begin with "To"; reword the block that mentions looseObject, strictObject, objectWithRest, and pojo so it flows better (e.g., combine clauses and vary openings), e.g., lead with a short summary about unknown entries then list alternatives using those schema names (`looseObject`, `strictObject`, `objectWithRest`, `pojo`) with one-sentence descriptions each; update the text in the same paragraph where the current sentence starting "To include unknown entries..." appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@website/src/routes/api/`(schemas)/object/index.mdx:
- Line 36: The explanation paragraph is choppy because multiple consecutive
sentences begin with "To"; reword the block that mentions looseObject,
strictObject, objectWithRest, and pojo so it flows better (e.g., combine clauses
and vary openings), e.g., lead with a short summary about unknown entries then
list alternatives using those schema names (`looseObject`, `strictObject`,
`objectWithRest`, `pojo`) with one-sentence descriptions each; update the text
in the same paragraph where the current sentence starting "To include unknown
entries..." appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab11dc19-d008-4123-a6f9-09a14060ede3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
library/package.jsonlibrary/src/schemas/index.tslibrary/src/schemas/pojo/index.tslibrary/src/schemas/pojo/pojo.test-d.tslibrary/src/schemas/pojo/pojo.test.tslibrary/src/schemas/pojo/pojo.tslibrary/src/schemas/record/record.test.tslibrary/src/schemas/record/record.tslibrary/src/schemas/record/recordAsync.test.tslibrary/src/schemas/record/recordAsync.tslibrary/src/utils/index.tslibrary/src/utils/isPlainObject/index.tslibrary/src/utils/isPlainObject/isPlainObject.test.tslibrary/src/utils/isPlainObject/isPlainObject.tslibrary/tsconfig.jsonwebsite/src/routes/api/(schemas)/looseObject/index.mdxwebsite/src/routes/api/(schemas)/object/index.mdxwebsite/src/routes/api/(schemas)/objectWithRest/index.mdxwebsite/src/routes/api/(schemas)/pojo/index.mdxwebsite/src/routes/api/(schemas)/pojo/properties.tswebsite/src/routes/api/(schemas)/record/index.mdxwebsite/src/routes/api/(schemas)/strictObject/index.mdxwebsite/src/routes/api/(types)/PojoIssue/index.mdxwebsite/src/routes/api/(types)/PojoIssue/properties.tswebsite/src/routes/api/(types)/PojoSchema/index.mdxwebsite/src/routes/api/(types)/PojoSchema/properties.tswebsite/src/routes/api/menu.mdwebsite/src/routes/guides/(main-concepts)/schemas/index.mdxwebsite/src/routes/guides/(schemas)/objects/index.mdx
✅ Files skipped from review due to trivial changes (18)
- library/src/utils/index.ts
- website/src/routes/api/(schemas)/looseObject/index.mdx
- library/src/schemas/pojo/index.ts
- library/src/utils/isPlainObject/index.ts
- library/tsconfig.json
- website/src/routes/api/(schemas)/record/index.mdx
- website/src/routes/api/(schemas)/strictObject/index.mdx
- website/src/routes/api/(types)/PojoIssue/index.mdx
- website/src/routes/guides/(schemas)/objects/index.mdx
- website/src/routes/api/(types)/PojoIssue/properties.ts
- website/src/routes/api/(types)/PojoSchema/index.mdx
- website/src/routes/api/menu.md
- website/src/routes/api/(schemas)/pojo/index.mdx
- website/src/routes/api/(types)/PojoSchema/properties.ts
- library/src/schemas/pojo/pojo.test-d.ts
- website/src/routes/api/(schemas)/pojo/properties.ts
- library/package.json
- library/src/utils/isPlainObject/isPlainObject.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- library/src/schemas/index.ts
- library/src/schemas/record/recordAsync.ts
- website/src/routes/api/(schemas)/objectWithRest/index.mdx
- website/src/routes/guides/(main-concepts)/schemas/index.mdx
- library/src/schemas/record/record.ts
- library/src/schemas/record/record.test.ts
- library/src/utils/isPlainObject/isPlainObject.ts
- library/src/schemas/pojo/pojo.test.ts
- library/src/schemas/record/recordAsync.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on the utility used by Redux, among other libraries:
This util correctly identifies POJOs (including null prototypes and POJOs from other realms) vs complex objects like arrays.
fixes #1387 (technically breaking, though arguably a bug fix to align with existing types)