Prepare for fastify v5 #129
Conversation
lib/filter-schema.js
Outdated
| app.graphql.addHook('preExecution', filterGraphQLSchemaHook.bind(app)) | ||
| if (!err) { | ||
| app.graphql.addHook('preExecution', filterGraphQLSchemaHook.bind(app)) | ||
| } |
There was a problem hiding this comment.
Are you sure about this? Isn't it swallowing the error?
There was a problem hiding this comment.
Yeah I thought the same and it's related to the issue I was coming up against! If I leave as it, app.ready() rejects with the expected error but then the error is thrown again within this callback and causes unhandled rejection issues.
Even though we're swallowing the error here, it's still being thrown and eventually surfaced, as shown and caught in this test here: https://github.com/mercurius-js/auth/blob/main/test/introspection-filter.js#L359
wdyt? Reckon it's worth looking for a better solution?
There was a problem hiding this comment.
Hey @mcollina ! Hope the Fastify release went okay this week! Just wondered what your thoughts were on my response to your comment? :)
There was a problem hiding this comment.
I've actually refactored it to use the fastify onReady hook instead, lemme know what you think!
Edit: Just double checking when this hook actually runs
There was a problem hiding this comment.
Yep, it should be okay from what I've read and the tests cover it and all run absolutely fine: https://github.com/mercurius-js/auth/blob/main/test/introspection-filter.js#L104
The only things I can think of with changing to use the onReady hook:
- It does fire slightly earlier that waiting until the
app.readycallback to fire but crucially, it still ensures that it fires the filter schema hook after all the auth context hooks are called. - If another
onReadyhook was registered after the plugin registration, this this would run after (rather than before with the original implementation) but I would argue that in this case, this introduces dependencies and you could get around it by registering theonReadybefore the plugin registration
wdyt?
cc. @Eomm for visibility as I think you did the original implementation of this :)
There was a problem hiding this comment.
Need to check - I don't remember ATM
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
|
@jonnydgreen can you bump the minimum Node.js version in CI to 20+? |
|
Yep, will do! |
|
Releasing a semver major version bump now! |
This includes the following changes