feat(types)!: remove TypeBox schema type inference support#179
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 53 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an exported ChangesEditableUser Type System Refinement
Possibly Related PRs
Suggested Labels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/core/src/@types/utility.ts (1)
39-41: 💤 Low valueConsider the safety implications of mapping all User keys to
any.The
EditableUsertype maps every key ofUsertoany, which completely removes type safety for all user fields. This permissive approach could lead to runtime errors if field types are misused.While this may be intentional for maximum flexibility in custom identity configurations, consider whether a more constrained approach (e.g.,
unknowninstead ofany, or preserving some base field types) would better balance flexibility and safety.Alternative: Use `unknown` for better type safety
export type EditableUser = { - [K in keyof User]: any + [K in keyof User]: unknown }This would require explicit type assertions, providing a bit more safety than
any.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/`@types/utility.ts around lines 39 - 41, EditableUser currently maps every property of User to any (export type EditableUser = { [K in keyof User]: any }), which removes type safety; change the mapped type to be safer—e.g., use unknown instead of any (export type EditableUser = { [K in keyof User]: unknown }) or selectively preserve core field types (keep explicit types for critical keys and map the rest to unknown) so callers must assert/validate values; update references to EditableUser accordingly to handle the stricter type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/`@types/utility.ts:
- Line 58: The TypeboxShapeToObject type is incorrectly merging a TypeBox schema
S with the runtime User type (via Wrap<Merge<S, User>}) instead of converting
the schema to its inferred runtime shape; update TypeboxShapeToObject to use
TypeBox's Static<TObject<S>> (or equivalent Static conversion) for S before
merging with User so schema definitions like TOptional<TString> become runtime
types (e.g., string | undefined). Locate the TypeboxShapeToObject alias and
replace the Merge<S, User> usage with Merge<Static<TObject<S>>, User> (or the
project's alias for Static/TObject) so tests expecting role?: string are
restored while keeping Wrap and Merge intact.
- Line 2: The TypeBox runtime type inference was broken by removing Static;
re-add Static to the import list and update the
TypeboxShapeToObject/FromShapeToObject type aliases to return Static applied to
the TypeBox schema (use Static<TSchema> / Static<TObject<...>> or
Static<TProperties<...>> as appropriate) so that TypeboxShapeToObject and any
usages (TypeboxShapeToObject, FromShapeToObject, TProperties, TObject, TSchema)
resolve to actual runtime value types (e.g., string) instead of schema objects;
ensure the restored import includes Static from "typebox" and adjust the type
alias at the previous TypeboxShapeToObject definition to wrap the schema with
Static.
- Around line 79-81: Add a focused compile-time type test that forces the
`FromShapeToObject` branch `S extends User ? S : never` to be exercised: in
`packages/core/test/types.test-d.ts` add a d.ts-only test that creates an
identity/schema which resolves to `User` (or calls `createAuth`/uses
`FromShapeToObject` with `User`-shaped `Identity`) and asserts assignability or
equality (e.g., via a type-level `Expect<Equal<...>>` or by assigning the result
to a `User`-typed variable) so the compiler must pick the `S extends User`
branch; alternatively if you cannot produce such a test, remove the fallback
case in `FromShapeToObject` or add a brief comment documenting why the `S
extends User` branch is required.
In `@packages/core/src/shared/identity.ts`:
- Line 72: ReturnShapeType currently lacks a branch for EditableUser so
ReturnShapeType<EditableUser> resolves to never; update the ReturnShapeType<T>
conditional type to include an EditableUser => <appropriate return shape> branch
(matching how other identity variants are handled) so createIdentity can accept
and return the correct type for EditableUser; adjust the conditional chain in
ReturnShapeType to include the EditableUser case and ensure any related
overloads or usages of Identities and createIdentity align with that return
shape.
In `@packages/core/test/identity.test.ts`:
- Around line 191-195: The test failure is caused by Typebox type inference
being broken after removing Static from TypeboxShapeToObject in utility.ts;
restore Static usage in TypeboxShapeToObject so TypeBox schemas map to their
Static types, allowing createAuth to infer the identity type from Schema
(removing the need for the explicit <ExpectedIdentity> generic and the schema as
any cast). Update the TypeboxShapeToObject type alias/utility to reintroduce
Static (importing it from `@sinclair/typebox` if necessary), ensure any dependent
helpers/types reference the corrected TypeboxShapeToObject, and run the
identity.test.ts to confirm Schema is inferred correctly by createAuth without
casts.
In `@packages/core/test/types.test-d.ts`:
- Line 146: The tests are asserting schema descriptors instead of runtime value
types because TypeboxShapeToObject lost the Static mapping; restore the runtime
mapping by reintroducing TypeBox's Static when converting shapes (e.g., update
TypeboxShapeToObject/FromShapeToObject to wrap the TypeBox schema types with
Static) so inferred types become runtime types (string | undefined) and update
the failing assertions in packages/core/test/types.test-d.ts (e.g., change role:
Typebox.TOptional<Typebox.TString> expectations to role?: string | undefined) to
reflect the corrected runtime types.
---
Nitpick comments:
In `@packages/core/src/`@types/utility.ts:
- Around line 39-41: EditableUser currently maps every property of User to any
(export type EditableUser = { [K in keyof User]: any }), which removes type
safety; change the mapped type to be safer—e.g., use unknown instead of any
(export type EditableUser = { [K in keyof User]: unknown }) or selectively
preserve core field types (keep explicit types for critical keys and map the
rest to unknown) so callers must assert/validate values; update references to
EditableUser accordingly to handle the stricter type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2644399-081f-49b9-9a0d-93ddcb4b165f
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.jsonpackages/core/package.jsonpackages/core/src/@types/utility.tspackages/core/src/shared/identity.tspackages/core/test/identity.test.tspackages/core/test/types.test-d.ts
halvaradop
changed the title
halvaradop
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/`(core)/guides/schema-validation.mdx:
- Line 10: Fix the grammar and clarity in the schema-validation copy: replace
"its provided the default `Identity` schemas" with "each library provides the
default `Identity` schemas" (or "it provides" depending on context), change
"They schemas contains" to "These schemas contain", and correct any occurrence
of "limitation by the expensive nature" to "limited by their expensive nature"
(or "limited due to their expensive nature"); update the same phrasing
occurrences mentioned around lines 188-190 and ensure the sentence about default
fields reads "The schemas contain `sub`, `name`, `email`, and `image` by
default, but you can extend them with additional fields."
- Line 10: The paragraph incorrectly claims all listed libraries support
built-in type inference; update the text that describes the default Identity
schema so it only claims validation and type inference for libraries that
actually support it and explicitly exclude TypeBox (referencing the Identity
schema and TypeBox) — rephrase to something like: the default Identity schema
(sub, name, email, image) can be extended and will leverage built-in validation
and type inference in supported libraries, while noting that TypeBox does not
support direct inference here. Ensure the Identity reference remains and fix the
grammar ("They schemas contains" → correct wording).
- Line 113: The import statement duplicates the type names; update the import so
each symbol appears only once and use the TypeScript type import form for the
types referenced — e.g., keep UserIdentity and import UserFrom and SessionFrom
as types by changing the import that currently lists UserFrom and SessionFrom
twice to a single clause that reads UserIdentity and type UserFrom, type
SessionFrom (referencing the existing import statement and symbols UserIdentity,
UserFrom, SessionFrom).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d79dd04-7c30-47e9-a075-c444a316f62d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
docs/src/content/docs/(core)/guides/schema-validation.mdxpackages/core/package.jsonpackages/core/src/shared/identity.tspackages/core/test/identity.test.tspackages/core/test/types.test-d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/test/types.test-d.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
1 participant
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.
Description
This pull request optimizes type inference by reducing TypeBox
Staticusage, lowering TypeScript instantiations from 1,323,740 to 158,861. The excessive number of instantiations was caused by deeply nested type operations generated by TypeBox's type system.This investigation started after identifying performance bottlenecks in
@aura-stack/router, primarily caused by TypeBox'sStatictype, which is used to infer the types ofsearchParams,params, andbody. The issue was initially addressed in aura-stack-ts/router#51 and aura-stack-ts/router#52. However, after running diagnostics on Aura Auth, I found that the same type inference patterns were still generating a significant number of TypeScript instantiations.tsc --extendedDiagnostics --generateTrace ./trace npx @typescript/analyze-trace ./trace > hot-spots.txtThe analysis showed that the primary hotspot was
src/client/client.ts. The reason is that, in the previously mentioned Aura Router pull requests, some TypeBox-based inference remained because it did not have a measurable impact on the diagnostics at the time and was therefore not removed.Trace analysis:
To validate the impact, I excluded
client.tsfrom the diagnostics. This reduced the total number of TypeScript instantiations from 1,323,740 to 144,130, confirming that the remaining TypeBox inference in this file was responsible for most of the overhead.Diagnostics: