fix: set default value for route schema to match type #5395
Conversation
|
I personally prefer this to #5394. |
done |
yeah, I like it more as well since updating the type might annoy some people, and it's really just an edge case inside hooks |
| @@ -206,7 +206,7 @@ Object.defineProperties(Request.prototype, { | |||
| get: () => context.config | |||
| }, | |||
| schema: { | |||
| get: () => context.schema | |||
| get: () => context.schema ?? {} | |||
There was a problem hiding this comment.
Isn't this enough to not get undefined?
Nope it's the deprecated version, sorry!
| t.not(req.routeSchema, undefined) | ||
| t.not(req.routeOptions.schema, undefined) |
There was a problem hiding this comment.
TBH I don't like this default.
How do the user know if a schema has been set?
Now: if(req.routeSchema==null)
Then: if(req.routeSchema.id==null) + the overhead to create a new empty object
Why don't we fix the types instead?
There was a problem hiding this comment.
Reopened #5394, both PRs fix the issue I reported but you guys know best what other use cases to consider.
I go with whatever is decided here :)
There was a problem hiding this comment.
If we go with this in v4, I’d say right after we should revert this in v5 and merge your other (type) PR there
There was a problem hiding this comment.
If we go with this in v4, I’d say right after we should revert this in v5 and merge your other (type) PR there
sounds like a plan, do you want the revert of this to be part of the type PR then? (needs rebase on next first)
There was a problem hiding this comment.
@gurgunday @Eomm then we go with the other PR. Let's not land and revert.
|
Closing as #5394 has been merged to |
Alternative to #5394 which changes runtime behavior instead of updating type
Closes #5393
Checklist
npm run testandnpm run benchmark[ ] documentation is changed or addedand the Code of conduct