-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Allow standard schemas to validate endpoint values #4864
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
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b9dc923:
|
size-limit report 📦
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
FWIW, I really like where your head is at in the second example you proposed. build.query({
query: ({ id }: { id: number }) => `posts/${id}`,
resultSchema: postSchema
}) Input/request validation is probably out of scope for a data fetching solution anyway. If inaccurate types are an issue in someone's application, it should probably be addressed at the source of the problem, not as a last ditch effort right before request is placed. The CPU overhead of validating all inputs of all requests probably isn’t great either, especially if this is occurring on main thread. I definitely prefer the terminology |
true - |
Here is link to my initial idea pitch just so there is a paper trail: Will respond directly to this PR for now on for brainstorming. |
It might be overkill, but i'm really enticed by the idea of performing schema validation in a web worker (off main thread). I have seen brief main thread lag when validating massive payloads from server (10MB+). One of the projects I work on professionally is a Tableau-esque data viewer where user can view and interact with up to 500k geospatial results. One of my Attached below is my rough attempt. The web worker was usable via promise. Usage inside API slice
workerPromise.ts
workerLogic.ts
Getting web worker to work with a variety of build tooling could be a pain though, particularly this kind of thing. My example above is obviously for a different use case (transforming a response instead of validating a payload), but I figure is worth sharing. |
As for question about whether validation would occur before transformResponse or after: "I would imagine responseSchema validating what data came back from the server pre-transformResponse. If a transformResponse function is provided, perhaps the RTKQ endpoint will use the return type of their provided transformResponse function. If no transformResponse function is provided, the RTKQ endpoint return type will be whatever was provided as responseSchema." If |
Definitely not something that makes sense for us to look into, but we would just accept any standard schema compliant value, so you're welcome to make your own: declare function validateInWorker(input: unknown): Promise<StandardSchemaV1.Result<string>>
const workerSchema: StandardSchemaV1<string> = {
'~standard': {
version: 1,
vendor: "custom",
validate: validateInWorker
}
} 😄 |
All schemas are optional - the difficult thing with Currently I have it as The difficulty with that is that build.query({
query: ({ id }: { id: number }) => `posts/${id}`,
rawResponseSchema: postSchema
}) however, this doesn't seem like a huge issue to me, as an ideal state of affairs would be build.query({
query: ({ id }: { id: number }) => `posts/${id}`,
rawResponseSchema: postSchema,
transformResponse: (res /* inferred from rawResponseSchema */) => transformed // provides final data type
})
// or no transformResponse
build.query({
query: ({ id }: { id: number }) => `posts/${id}`,
responseSchema: postSchema,
}) but this isn't currently possible (the inference part at least). The closest you'd get is manually annotating build.query({
query: ({ id }: { id: number }) => `posts/${id}`,
rawResponseSchema: postSchema,
transformResponse: (res: Infer<typeof postSchema>) => transformed // provides final data type
}) |
cool, that wasn't actually too bad 😄 // without transformResponse
build.query({
query,
responseSchema
})
// with transformResponse
build.query({
query,
rawResponseSchema,
transformResponse
}) We could use a union to enforce this, but honestly the types are complicated enough and i think some people may still want to use both. |
Is it not possible due to limitations in standard schema specification, the way RTKQ is currently structured, or due to limitation in TypeScript language itself? |
Due to how it's currently structured - I've just pushed a change adding the inference :) |
Have’t thought too deeply about it, but agreed about not using union. Adding the ability to provide both rawResponseSchema and regular responseSchema may not a be common use-case, but probably wouldn't hurt to have. |
Ability to provide a global callback for situation where schema validation fails would be super valuable btw. Would allow for warning toast, logging to external service, etc. Callback would contain information about the request such as endpoint name, request payload, etc. Ability to enable/disable schema validation on global and per-endpoint basis may be nice too. Similar to how RTKQ has a global |
Added - callback parameters are |
b53b271
to
de309e9
Compare
@agusterodin can you give an example of when/why an option to skip schema validation would actually be necessary? Like, in that geospatial results blog example: do you really need to validate the entire response? when would you want to supply a schema for that endpoint and then not run it? not sure I follow the intended use case for such an option. is it just to get the type inference? "trust me, the data will look like this, just don't bother checking for me"? |
b85228a
to
ed567b7
Compare
…ird type parameter)
ed567b7
to
b9dc923
Compare
The rationale would be to still take advantage of type inference and so that endpoint definitions can be consistent. Mixing-and-matching providing standard schema vs. providing generics (the current way of defining request/response types) based solely on whether you want to validate payload or not would likely be clunky. More context on why we skip schema validation for certain endpoints: mostly for performance reasons. There are a few endpoints we use that return massive amounts of data (10MB+) and cause the browser to dramatically slow down when trying to validate the schema. For these rare cases, we leave the Zod schema in the code but comment it out. ![]() There are probably other ways to mitigate:
|
@agusterodin gotcha, thanks for the response. I'm planning to ship this in 2.7.0. Was going to release that today :) but realized we still need docs for this feature, plus a couple other tweaks. Could you give the current PR build a shot and give me any last-minute feedback before this goes live? |
Hell yeah! Will test and provide feedback over next couple days |
Have been playing around with PR build. I absolutely love how the standard schema integration is implemented! I have been testing it here https://github.com/agusterodin/rtkq-playground/tree/standard-schema. Provided this link in case it helps investigating anything Next.js specific (be sure you're on the Overall extremely thrilled with this feature. Here are some random observations: Seeing Schema errors bubble up to Next.js error dialog since they are uncaught (as shown in image above). Ideally RTK catches the error itself so that the Next.js error dialog doesn't get triggered. I much prefer displaying a warning toast message and opening the browser devtools console if I want to investigate the error. In my current Zod schema validation implementation (not the official RTK implementation in this PR), i'm able to avoid having the parse error "bubble up" to the Next.js error dialog by putting it in a try-catch block like this
I understand that other people may actually want this error to "bubble up" to the Next.js error dialog (possibly matter of personal preference). In that case, the error can be rethrown inside of the global
Is there any way to access/log the parsing error message produced by Zod? In the screenshot below I call These detailed Zod errors are one of the primary value-adds of data fetch schema validation in my opinion. They are extremely useful for both local development and issue triage. We frequently attach screenshots of these detailed Zod errors to Slack threads and bug tickets. In comparison, the only message I currently see in PR build is Not sure if this is RTKQ specific or is just the way Zod exposes errors for standard schema usages. I may not be grasping how My assumption is that it validates the JSON the server sends back (if any) when receiving a response with an error status code (400 or 500 series). Something like this (note that this is not how the PokeAPI formats errors, this is just a hypothetical):
And you would define it like this in your endpoint definition:
If I provide
Is there a way to simplify this so that you only need to provide the schema like this? From what I can tell, we don't have control over
Is there any way There may be a way to enhance this since we know what the error response schema should be (if schema validation succeeds). This may be relatively obscure, but how do we distinguish something as a "schema validation error" from I could imagine it looking something like this (obviously SCHEMA_ERROR isn't currently a thing):
Side note: would be awesome if RTKQ provided an out-of-the-box |
The root issue here is that with the way RTKQ is architected, there are only two types of error:
There's no such thing as a unique error type for an endpoint, for example, and adding this would be a major restructure of RTKQ's types. With regards to schemas, we have absolutely no way of knowing how to turn our schema errors into a shape that would match the base query's errors. We could possibly leave this up to the user: const api = createApi({
baseQuery: fetchBaseQuery(),
catchSchemaFailure: (error, info) => ({ status: "CUSTOM_ERROR", data: error.issues, error: `${error.schemaName} failed validation` })
endpoints: () => ({})
}) This still raises questions - should the schema error be passed to A more drastic approach would be adding a whole new type of error to the system specifically for schema failures, but this would be potentially breaking for code like the above that's only expecting All the reasons above is also why your errorResponseSchema needs to match the whole base query error, because we'd have no idea how to extract only the server response from the shape returned from the base query. None of our code can be written assuming that errors will only ever be |
@agusterodin i've raised #4934 to add |
Type-wise, schemas are required to match the type of the value they're validating - they can't change a string to a number for example. Transformations that preserve the correct type are allowed, however.
Schemas can also be used as a source of inference, for example
Schema parse failures are treated as unhandled errors, i.e. they're thrown and serialized rather than being returned. This is mainly because we have no idea what the base query error shape should look like, so we can't match it.
For transformable values,
raw<value>Schema
checks the value before transformation, and the<value>Schema
checks the possibly transformed value.