[plain-object] Prevent accidental static built-in classes (atomics, JSON, Math) to be passed#2615
[plain-object] Prevent accidental static built-in classes (atomics, JSON, Math) to be passed#2615belgattitude wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: c0e8079 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change updates type signatures to exclude static built-in classes (Math, JSON, Atomics) at compile time, introduces related utility types, deprecates isStaticBuiltInClass, adjusts exports, expands tests, and updates documentation and several package.json dependencies. Runtime behavior of isPlainObject remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant TS as TypeScript
participant Lib as @httpx/plain-object
Dev->>Lib: isPlainObject(Math)
note over Lib: Runtime path unchanged
Lib-->>Dev: true (runtime)
Dev->>TS: Type-check call
TS-->>Dev: Error TS2345 (Math is excluded by WithoutStaticBuiltInClass)
Dev->>Lib: isStaticBuiltInClass(Math)
Lib-->>Dev: true
note right of Lib: Deprecated API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many --target=build --exclude=examples/*... |
❌ Failed | 40s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-15 15:03:23 UTC
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![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 (5)
.changeset/cyan-chefs-agree.md (1)
5-5: Minor grammar fix suggested.The phrase "in next major version" should be "in the next major version" for grammatical correctness.
Apply this diff:
-Deprecate isStaticBuiltInClass, it will be removed in next major version +Deprecate isStaticBuiltInClass, it will be removed in the next major version.changeset/weak-onions-walk.md (2)
8-8: Minor grammar fix suggested.The phrase "passed as argument" should be "passed as an argument" for grammatical correctness.
Apply this diff:
-if one of these classes is passed as argument. +if one of these classes is passed as an argument.
10-10: Minor grammar fix suggested.When used as a compound adjective modifying "code", "real world" should be hyphenated as "real-world".
Apply this diff:
-Note that it's totally an edge case that probably never happens in real world code. +Note that it's totally an edge case that probably never happens in real-world code.packages/plain-object/src/is-plain-object.ts (1)
57-70: Clever use of generics to enforce type-level constraints.The type signature leverages intersection types to prevent static built-in classes:
TAnyInput & WithoutStaticBuiltInClass<TAnyInput>evaluates toneverwhenTAnyInputextendsStaticBuiltInClass- This triggers a TypeScript error without runtime overhead
- The return type predicate maintains type information for valid inputs
One suggestion: Consider adding a JSDoc comment on the
TAnyInputgeneric parameter explaining that it's auto-inferred and users should not set it manually, to prevent confusion.Example enhancement:
export const isPlainObject = < /** Custom type of the plain object values, DefaultBasePlainObject by default */ TValue extends BasePlainObject = DefaultBasePlainObject, /** - * Do not set second generic, its purpose is to prevent allowing static - * built-in classes like Math, JSON, Atomics + * @internal Auto-inferred input type. Do not set manually. + * Used to enforce compile-time exclusion of static built-in classes + * (Math, JSON, Atomics) via WithoutStaticBuiltInClass constraint. */ TAnyInput = unknown,packages/plain-object/README.md (1)
125-125: Minor: Consider using "until" instead of "till" for formality.While "till" is grammatically correct, "until" is more commonly used in technical documentation and appears more formal.
Apply this diff:
-> info: Since v2.0.0 and deprecated till v3.0.0 +> info: Since v2.0.0 and deprecated until v3.0.0Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.changeset/cyan-chefs-agree.md(1 hunks).changeset/weak-onions-walk.md(1 hunks)devtools/vitest/package.json(1 hunks)docs/package.json(1 hunks)docs/src/pages/plain-object.mdx(2 hunks)examples/nextjs-app/package.json(2 hunks)package.json(1 hunks)packages/plain-object/README.md(3 hunks)packages/plain-object/src/__tests__/types.test.ts(1 hunks)packages/plain-object/src/assert-plain-object.ts(2 hunks)packages/plain-object/src/index.ts(1 hunks)packages/plain-object/src/is-plain-object.ts(2 hunks)packages/plain-object/src/is-static-built-in-class.ts(1 hunks)packages/plain-object/src/static-built-in-class.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/plain-object/src/is-static-built-in-class.ts (2)
packages/plain-object/src/index.ts (1)
StaticBuiltInClass(6-6)packages/plain-object/src/static-built-in-class.types.ts (1)
StaticBuiltInClass(4-4)
packages/plain-object/src/__tests__/types.test.ts (1)
packages/plain-object/src/is-plain-object.ts (1)
isPlainObject(57-82)
packages/plain-object/src/static-built-in-class.types.ts (2)
packages/plain-object/src/index.ts (2)
StaticBuiltInClass(6-6)WithoutStaticBuiltInClass(7-7)packages/plain-object/src/is-static-built-in-class.ts (1)
StaticBuiltInClass(4-4)
packages/plain-object/src/assert-plain-object.ts (3)
packages/plain-object/src/internal.types.ts (3)
BasePlainObject(3-3)DefaultBasePlainObject(5-7)MsgOrErrorFactory(20-20)packages/plain-object/src/static-built-in-class.types.ts (1)
WithoutStaticBuiltInClass(9-11)packages/plain-object/src/plain-object.types.ts (1)
PlainObject(9-13)
packages/plain-object/src/is-plain-object.ts (4)
packages/assert/src/object.guards.ts (1)
isPlainObject(96-114)packages/plain-object/src/internal.types.ts (2)
BasePlainObject(3-3)DefaultBasePlainObject(5-7)packages/plain-object/src/static-built-in-class.types.ts (1)
WithoutStaticBuiltInClass(9-11)packages/plain-object/src/plain-object.types.ts (1)
PlainObject(9-13)
🪛 GitHub Actions: CD-pkg-pr-new
package.json
[warning] Unknown: No additional issues detected in this log segment.
docs/package.json
[warning] Unknown: No additional issues detected in this log segment.
devtools/vitest/package.json
[warning] Unknown: No additional issues detected in this log segment.
examples/nextjs-app/package.json
[warning] Unknown: No additional issues detected in this log segment.
🪛 LanguageTool
.changeset/cyan-chefs-agree.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...sStaticBuiltInClass, it will be removed in next major version
(QB_NEW_EN)
.changeset/weak-onions-walk.md
[grammar] ~8-~8: There might be a mistake here.
Context: ...rror if one of these classes is passed as argument. Note that it's totally an e...
(QB_NEW_EN)
[grammar] ~10-~10: Use a hyphen to join words.
Context: ...case that probably never happens in real world code. ```typescript import { isPl...
(QB_NEW_EN_HYPHEN)
packages/plain-object/README.md
[style] ~125-~125: Consider using the more formal “until”.
Context: ...ss > info: Since v2.0.0 and deprecated till v3.0.0 Since v2.0.0, isPlainObject w...
(TILL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: test-examples (20.x)
- GitHub Check: test-packages (24.x)
- GitHub Check: test-packages (22.x)
🔇 Additional comments (15)
devtools/vitest/package.json (1)
48-48: LGTM!The dependency updates to
vite(patch bump) and@types/react-dom(type definitions update) align with the repository-wide synchronization of dependencies.Also applies to: 55-55
docs/package.json (1)
28-28: LGTM!The type definitions update for
@types/react-domis consistent with similar updates across the repository.package.json (1)
89-90: LGTM!The dev dependency updates (TypeScript ESLint packages and npm-check-updates) are minor patch bumps with no breaking changes.
Also applies to: 97-97
examples/nextjs-app/package.json (1)
36-38: LGTM!The dependency updates include minor/patch bumps for runtime dependencies (
hono,ky,next) and dev dependencies (@belgattitude/eslint-config-bases,@types/react-dom,eslint-config-next,vite). All updates appear safe and align with the repository-wide dependency synchronization.Also applies to: 48-48, 52-52, 58-58, 65-65
packages/plain-object/src/is-static-built-in-class.ts (1)
1-3: LGTM!The deprecation markers are appropriate given that this functionality is being deprecated in favor of type-level guards.
Also applies to: 6-8
.changeset/weak-onions-walk.md (1)
17-18: Verify the error message reflects the corrected type definition.The example shows the TypeScript error as
Argument of type Math | JSON | Atomics, but this should betypeof Math | typeof JSON | typeof Atomicsonce the type definition inis-static-built-in-class.tsis corrected. Please verify and update the error message after fixing the type definition.docs/src/pages/plain-object.mdx (2)
98-106: LGTM!The added documentation clearly explains the distinction between runtime behavior (returns true for static built-in classes) and compile-time type guards (prevents passing them). This is an important clarification for users and appropriately emphasizes the performance trade-off.
258-268: LGTM!The compatibility table updates reflect current Node versions and browser support accurately.
packages/plain-object/src/__tests__/types.test.ts (1)
42-53: LGTM! Excellent test coverage for the new type-level guard.The parameterized test effectively verifies that:
- TypeScript prevents passing static built-in classes at compile time (
@ts-expect-error)- Runtime behavior still returns
truefor these values (intentional trade-off for performance)This test precisely captures the dual behavior documented in the PR objectives.
packages/plain-object/src/static-built-in-class.types.ts (1)
1-11: Well-designed type utilities for static built-in class filtering.The type definitions are clean and correct:
StaticBuiltInClassproperly usestypeofto represent constructor typesWithoutStaticBuiltInClass<T>correctly excludes these classes via conditional typing- Clear JSDoc comments explain the purpose
This centralized type definition file provides a solid foundation for the type-level guards applied throughout the codebase.
packages/plain-object/src/is-plain-object.ts (1)
47-55: Clear documentation of runtime vs. compile-time behavior.The JSDoc update effectively communicates the intentional trade-off:
- Runtime acceptance of static built-in classes (for performance)
- Type-level prevention via typing guards (for safety)
This transparency helps developers understand why they might see type errors and the reasoning behind the design decision.
packages/plain-object/README.md (1)
80-89: Excellent documentation of the type-level guard behavior.The added warning section clearly explains:
- Runtime behavior (returns
truefor static built-in classes)- Type-level prevention (TypeScript errors)
- The rationale (performance vs. safety trade-off)
This helps users understand why they might encounter type errors and sets appropriate expectations.
packages/plain-object/src/index.ts (1)
5-8: Clean consolidation of type exports.The updated export structure properly centralizes static built-in class types from
static-built-in-class.types.ts, making them available as part of the public API. This organization improves maintainability and discoverability.packages/plain-object/src/assert-plain-object.ts (2)
43-43: Good addition of @throws documentation.The added
@throwsJSDoc tag improves API documentation by explicitly stating the error behavior. This helps TypeScript language servers provide better hover information.
45-59: Consistent implementation with isPlainObject.The function signature updates mirror those in
isPlainObject, maintaining consistency across the API:
- Same generic pattern (
TValue,TAnyInput)- Same constraint (
WithoutStaticBuiltInClass<TAnyInput>)- Matching assertion signature
This consistency ensures uniform type-level protection against static built-in classes throughout the library.
| /** | ||
| * @deprecated | ||
| */ | ||
| export type StaticBuiltInClass = Math | JSON | Atomics; |
There was a problem hiding this comment.
Fix the type definition - missing typeof keyword.
The type definition is incorrect. It should use typeof Math | typeof JSON | typeof Atomics instead of Math | JSON | Atomics. Without typeof, the type refers to instance types rather than the actual static built-in class objects.
Apply this diff:
-export type StaticBuiltInClass = Math | JSON | Atomics;
+export type StaticBuiltInClass = typeof Math | typeof JSON | typeof Atomics;This aligns with the correct definition in packages/plain-object/src/static-built-in-class.types.ts (line 3) as shown in the relevant code snippets.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type StaticBuiltInClass = Math | JSON | Atomics; | |
| export type StaticBuiltInClass = typeof Math | typeof JSON | typeof Atomics; |
🤖 Prompt for AI Agents
In packages/plain-object/src/is-static-built-in-class.ts around line 4, the type
alias uses instance types (Math | JSON | Atomics) but should refer to the
constructor/static objects; change the definition to use typeof for each (i.e.,
typeof Math | typeof JSON | typeof Atomics) so it matches
packages/plain-object/src/static-built-in-class.types.ts and correctly types the
static built-in class objects.
1 participant
Summary by CodeRabbit