-
-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose an option to customise schema combination #313
base: main
Are you sure you want to change the base?
Conversation
…n option to customise final schema
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@EskiMojo14 is attempting to deploy a commit to the t3-oss Team on Vercel. A member of the Team first needs to authorize it. |
Co-authored-by: Julius Marminge <[email protected]>
649e26e
to
e92c15b
Compare
- `CreateEnv` now has the signature `CreateEnv<TFinalSchema, TExtends>`, instead of the previous `CreateEnv<TServer, TClient, TShared, TExtends>`. | ||
- Previous behaviour can be achieved by using `DefaultCombinedSchema<TServer, TClient, TShared>` as the type for `TFinalSchema`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a user-breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateEnv is exported, so technically yes - in practicality I doubt anyone was relying on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's exported cause the other packages needs it. I should add some @internal jsdoc annotations to things
@@ -629,3 +629,115 @@ describe("extending presets", () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe("createFinalSchema", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work with extends
? can you add some tests for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do - extends just assigns all of the properties to the final result from the schema, like it did before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I was thinking of was more like what if you do some combinator that accesses server stuff, will that cause an "invalid server access" or not? I'd think not since you're not giving the proxy as the argument there right? also what are the types in that callback like, server variables will be undefined when the combinator runs on the client etc etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #240, the whole Proxy thing doesn't really work properly with extends
- but that's irrelevant to createFinalSchema, because the proxy isn't applied until the schema is finished validating.
As for types, you're correct - it's currently typed to what it'd be on the server, like the rest of the library (client + server + shared). I don't know how much it would complicate things to mark the server variables as optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schemas have always been allowed to access any key they want
No since accessing a server var on the client would throw an error - so e.g if you'd chain on a toLower()
you'd get the invalid access before. It seems here we'd get a "Cannot access property toLower of undefined" which feels suboptimal - but maybe I'm overthinking it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
schemas have never been run on the output of presets, they were always run before it. they have always been passed the raw runtimeEnv option, and any presets were merged into the parsed result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the concern, but I don't think there's a good way to expose type-wise that the server variables might not be defined to the createFinalSchema
callback. Best case, the schema result also reflects the server variables as optional (whereas they're currently always defined), worst case the inference breaks completely. We also can't wrap the intermediate parsed value in a Proxy since we don't have access to it.
We could possibly pass some sort of makeSafe
function to the user that would make a Proxy, but that would need to be opt in.
createObjectSchema: (shape, { isServer, makeSafe }) => z.object(shape).refine((_env) => {
const env = makeSafe(_env) // wraps in Proxy
if (!isServer) env.SERVER_ENV // throws
// ...
})
>(); | ||
expect(env).toMatchObject({ SKIP_AUTH: true }); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated, might be time to split this file up soon 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be an interesting idea to consolidate the tests, just passing different schema definitions when calling
runSmokeTest({ foo: v.string() });
runSmokeTest({ foo: z.string() });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors are reported a bit differently so would need to take that into account but yea, definitely should have some shared tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliusmarminge what about something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made ^ into a library for fun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! feel free to use it here :) - can be a follow-up pr
This allows extension like refinement and transformation.
fixes #268
alternative approach to #169