-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
updated fastify plugin to work with fastify v5 #2669
Conversation
Reviewer's Guide by SourceryThis pull request updates the Altair Fastify plugin to work with Fastify v5. The main changes involve modifying the plugin structure, updating type imports, and adjusting the export mechanism. An example for Fastify v5 usage has also been added. Sequence diagram for Fastify v5 plugin registrationsequenceDiagram
actor User
participant FastifyApp as Fastify Application
participant AltairFastify as AltairFastify Plugin
User->>FastifyApp: Start application
FastifyApp->>AltairFastify: Register plugin
AltairFastify-->>FastifyApp: Plugin registered
FastifyApp->>User: Server listening at http://localhost:3000
Updated class diagram for Altair Fastify PluginclassDiagram
class AltairFastifyPluginOptions {
path: string
baseURL: string
endpointURL: string
}
class fastifyAltairPluginFn {
+fastify: FastifyInstance
+AltairFastifyPluginOptions
}
class AltairFastify {
+fastify: ">= 3.x"
+name: "altair-fastify-plugin"
}
AltairFastifyPluginOptions <|-- fastifyAltairPluginFn
fastifyAltairPluginFn <|-- AltairFastify
AltairFastifyPluginOptions : RenderOptions
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding basic error handling in the new example file (examples/fastify-v5/index.ts) for server startup.
- Please ensure that the documentation is updated to reflect the changes for Fastify v5 compatibility.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Visit the preview URL for this PR (updated for commit 5b0a193): https://altair-gql--pr2669-imolorhe-support-fas-p7fv048u.web.app (expires Sat, 19 Oct 2024 12:49:06 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
@imolorhe Better to follow the standard fastify plugin syntax: export const AltairFastify = fp<AltairFastifyPluginOptions>(
async (
fastify,
{
baseURL = "/altair/",
endpointURL = "/graphql",
path = "/altair/",
...renderOptions
},
) => {
// logic here
},
{
fastify: ">= 3.x",
name: "altair-fastify-plugin",
},
);
export default AltairFastify; |
@xoldd fastify plugins can't use named exports? |
They can. I've used a named export. I also used a default export because many fastify plugins do that. Edit: Just, noticed the |
Yeah the default export isn't necessary. I don't see how it would be a problem but you can skip it if you want to. Everything I've shown in the syntax is just the more cleaner way of doing it, it wouldn't make a difference in functionality compared to how you've done it. |
Fixes
Closes #2663
Checks
yarn test-build
Changes proposed in this pull request:
Summary by Sourcery
Update the Altair Fastify plugin to support Fastify v5 by modifying the plugin function signature and adding an example for Fastify v5 usage.
New Features:
Enhancements: