fix: set default value for route schema to match type#5395
fix: set default value for route schema to match type#5395nflaig wants to merge 3 commits intofastify:mainfrom
Conversation
mcollina
left a comment
There was a problem hiding this comment.
Can you add a test for this change?
|
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 |
| }, | ||
| 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!
gurgunday
left a comment
There was a problem hiding this comment.
lgtm
I think in next we should fix the type instead and it should be undefined
| 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 |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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