refactor: replace JsonObject with custom EntityBase type#4
Conversation
Replace type-fest's JsonObject with custom ComponentValue + EntityBase types. EntityBase includes `| undefined` in its index signature so TypeScript correctly models missing components, eliminating the no-unnecessary-condition eslint override. ComponentValue still prevents functions/symbols as component data. - Drop type-fest dependency (saves consumers 512KB) - Remove no-unnecessary-condition eslint override (no longer needed) - Remove duplicate SafeEntity type from archetype.ts - Zero runtime dependencies Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for objecs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughTightened type constraints by introducing exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/objecs/src/world.ts (1)
302-328:⚠️ Potential issue | 🟡 Minor
addEntityComponentssingle-component branch silently falls through whenvalueisundefined.Line 315:
typeof componentOrComponents === 'string' && value !== undefined— if a caller passes a string key withvalue === undefined(e.g. a runtime slip), execution falls into theelsebranch and treats the string key as a components map, silently no-oping. The TypeScript overload typesvalueasNonNullable<Entity[Component]>, so this can't happen at compile time, but it's a runtime footgun. Consider a guard or an explicit assertion:🛡️ Proposed guard
if (typeof componentOrComponents === "string" && value !== undefined) { // `@ts-ignore` entity[componentOrComponents] = value; changedKeys.push(componentOrComponents as keyof Entity); -} else { +} else if (typeof componentOrComponents !== "string") { const components = componentOrComponents as Record<string, unknown>; for (const key in components) { (entity as any)[key] = components[key]; changedKeys.push(key as keyof Entity); } +} else { + // string key with undefined value — no-op (component already absent or value intentionally omitted) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/objecs/src/world.ts` around lines 302 - 328, The single-component branch in addEntityComponents can silently fall through when componentOrComponents is a string but value === undefined; add an explicit runtime guard at the start of that branch to detect this case and throw a clear Error (or assert) instead of treating the string as a components map—ensure the guard checks typeof componentOrComponents === "string" and value === undefined, throws (e.g. "Missing value for component '<key>'"), and leaves changedKeys handling unchanged so single-component updates still push the key when valid.
🧹 Nitpick comments (1)
packages/objecs/src/world.ts (1)
3-19: Well-structured — mirrorsJsonValue/JsonObjectfrom type-fest exactly as intended.One subtle behaviour worth documenting in the JSDoc: TypeScript 4.2 allows objects with optional properties to assign to index signatures whose value type doesn't include
undefined, so{ trigger?: string }satisfies{ [key: string]: ComponentValue }— but a non-optional property typed asComponentValue | undefinedexplicitly would still fail. Since this distinction surprises contributors, a brief note in theComponentValueJSDoc would prevent confusion.📝 Suggested JSDoc addition
/** * Allowed component value types. Prevents functions, symbols, and other * non-serializable values from being used as component data. + * + * Nested object properties with the optional modifier (`prop?`) satisfy this + * constraint (TypeScript 4.2+). Properties typed as `T | undefined` explicitly + * (without `?`) do not — use `prop?: T` instead. */ export type ComponentValue =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/objecs/src/world.ts` around lines 3 - 19, Add a brief JSDoc note to the ComponentValue type (and optionally reference EntityBase) explaining the TypeScript 4.2 nuance: objects with optional properties (e.g., { trigger?: string }) are assignable to an index signature whose value type does not include undefined, whereas a property explicitly typed as ComponentValue | undefined is not the same; update the comment above ComponentValue to call out this difference so contributors understand why optional props behave differently in type compatibility checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/objecs/src/world.ts`:
- Around line 302-328: The single-component branch in addEntityComponents can
silently fall through when componentOrComponents is a string but value ===
undefined; add an explicit runtime guard at the start of that branch to detect
this case and throw a clear Error (or assert) instead of treating the string as
a components map—ensure the guard checks typeof componentOrComponents ===
"string" and value === undefined, throws (e.g. "Missing value for component
'<key>'"), and leaves changedKeys handling unchanged so single-component updates
still push the key when valid.
---
Nitpick comments:
In `@packages/objecs/src/world.ts`:
- Around line 3-19: Add a brief JSDoc note to the ComponentValue type (and
optionally reference EntityBase) explaining the TypeScript 4.2 nuance: objects
with optional properties (e.g., { trigger?: string }) are assignable to an index
signature whose value type does not include undefined, whereas a property
explicitly typed as ComponentValue | undefined is not the same; update the
comment above ComponentValue to call out this difference so contributors
understand why optional props behave differently in type compatibility checks.
jakeklassen
force-pushed
the
refactor/entity-base-type
branch
from
34dc400 to
0ac352c
Compare
Verify that functions and symbols are rejected as component values at compile time, and that valid JSON-compatible types are accepted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/objecs/src/world.test.ts`:
- Line 353: Replace the deprecated assertion: change the expectTypeOf check that
uses toMatchTypeOf to the new toExtend variant; specifically update the
assertion on ValidEntity and EntityBase
(expectTypeOf<ValidEntity>().toMatchTypeOf<EntityBase>()) to use
expectTypeOf<ValidEntity>().toExtend<EntityBase>() so the type-assignability
test uses the supported matcher.
Response to CodeRabbit review1. Disagree. The TypeScript overloads type 2. JSDoc note about Disagree. This is standard TypeScript behavior (TS 4.2 index signature compatibility), not specific to our types. Every example and test in the library uses the 3. Already fixed in |
1 participant
Summary
type-fest'sJsonObjectwith customComponentValue+EntityBasetypesEntityBase = Record<string, ComponentValue | undefined>— the| undefinedmakes TypeScript correctly model missing componentsComponentValuestill prevents functions/symbols/non-serializable values as component data (same safety asJsonObject)type-festfrom dependencies — zero runtime dependencies (saves consumers 512KB)@typescript-eslint/no-unnecessary-condition: "off"eslint override — no longer needed since TS now knows property access can beundefinedSafeEntitytype fromarchetype.ts(imports fromworld.tsinstead)Why
JsonObject's index signature{[Key in string]: JsonValue}tells TypeScript every property access returnsJsonValue— neverundefined. But at runtime, missing ECS components AREundefined. This mismatch required an eslint rule override and obscured the type system's understanding of our code.The new
EntityBasetype accurately models the runtime behavior while maintaining the same "no functions as components" constraint.Test plan
pnpm --filter objecs build— lint + build passespnpm --filter objecs test -- --run— all 22 tests passpnpm lint— full monorepo lint cleanpnpm --filter objecs check-exports— all export conditions green🤖 Generated with Claude Code
Summary by CodeRabbit