Skip to content
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

Fastify plugins integration #1220

Closed
2 tasks done
madflow opened this issue Jan 25, 2020 · 13 comments · Fixed by #1361
Closed
2 tasks done

Fastify plugins integration #1220

madflow opened this issue Jan 25, 2020 · 13 comments · Fixed by #1361
Labels
🔁 revisit-in-v5 Not happy with this behaviour, but changing it is a breaking change.

Comments

@madflow
Copy link
Contributor

madflow commented Jan 25, 2020

I'm submitting a ...

  • bug report
  • feature request

PostGraphile version: latest

Hi!

This was triggered by: #789 (comment)

Fastify uses a plugin system for registering plugins like fastify-compress or fastify-helmet.

Also there is a Express middleware compatible use integration.

I mount Postgraphile as a middleware.

Some Fastify plugins seem to work with this setup (fastify-helmet) and some do not: for example fastify-compress.

However - it is possible to achieve compression by use the (Express!) compression middleware.

This was my testcase:

// yarn add postgraphile fastify fastify-helmet fastify-compress compression
// DATABASE_URL=... node server.js

const { postgraphile } = require("postgraphile");
const fastifyHelmet = require("fastify-helmet");
const expressCompression = require('compression');
const fastifyCompression = require('fastify-compress');

const Fastify = require('fastify');

const app = Fastify({
  logger: true,
});

// This is working
app.register(fastifyHelmet);

// app.use(expressCompression({threshold: 0})); // working

// app.register(fastifyCompression, {threshold: 0}); // not working

app.use(
  postgraphile(process.env.DATABASE_URL, 'public', {
    graphiql: true,
    enhanceGraphiql: true
  })
);

app.listen(process.env.PORT || 3000, err => {
  if (err) {
    app.log.error(err);
    process.exit(1);
  }
});

Hopefully I did everything correct here.

I would aplaude tighter coupling of Fastify (and plugin system) and Postgraphile.

From what I gather is that the Koa compression issues has been fixed my some Koa specific "hacks". Maybe this would suffice for Fastify, too? Or - should just Express middleware used with the Fastify compat layer?

I can provide a PR with a failing test for the fastify-compress issue if desired.

@madflow
Copy link
Contributor Author

madflow commented Apr 21, 2020

Just a fyi: We switched back to Express :S . So this issue and the PR #1221 is not critical/important to "me" anymore. Maybe it is important for others - so I am not closing this issue or the PR if not desired.

@benjie benjie added the 🔁 revisit-in-v5 Not happy with this behaviour, but changing it is a breaking change. label Sep 28, 2020
@alemagio
Copy link

Hi
don't know if someone might still want it but I recently released a plugin to integrate postgraphile with fastify.
No need to use middlewares since they are not supported natively by fastify 3.x.
Still working on it but if you need or want to help you can find it here

@benjie
Copy link
Member

benjie commented Oct 21, 2020

Hi @alemagio; perhaps you’d like to test #1361 which we’re planning to merge soon. I’d love to know if it fully addresses fastify support.

@alemagio
Copy link

Hi @benjie
Sure, I just created it for my own project but I'm willing to improve it and that PR looks a great starting point.
Thanks

@benjie
Copy link
Member

benjie commented Oct 21, 2020

Our fastify tests are working with basically this:

const { postgraphile } = require('postgraphile');
const fastify = require('fastify');

const middleware = postgraphile(...);
const app = fastify(...);
app.use(middleware);
app.listen(3000);

Would you like to set up a more complex example (e.g. with plugins, other middleware, etc) and see if it works? Instructions on how to install a pre-merged PR here: #1361 (comment) It'd be really good to get some more confirmation.

@alemagio
Copy link

alemagio commented Oct 21, 2020

Standing to fastify docs middlewares in v3 will need an additional plugin to work.
Nothing hard to do but since fastify works with plugins I'm trying to create one without using fastify.use.

I'll test my project with #1361 as soon as possible (hopefully by the end of the week)
Thanks for your help 👍

@benjie
Copy link
Member

benjie commented Oct 21, 2020

Ah; if middlewares are no longer supported, #1361 also adds a broken down version; I'll have a go at getting that working. Apparently our tests are still on fastify v2! Thanks for the heads up.

@alemagio
Copy link

@benjie
I've created this repo using the branch of #1361
I've used a plugin called middie to allow the use of middlewares in fastify 3.
The system looks fine, the schema is generated but I don't get the default route at /graphql, I think the problem is related to how Fastify handle routes or maybe I did something wrong 😅

@benjie
Copy link
Member

benjie commented Oct 21, 2020

Interesting; I think a middie-based approach would give users the easiest path - the whole app.use(postgraphile(...)) pattern is well liked!

Not sure if it helps regarding your issue, but I've created a Fastify v3 adaptor that's working with our tests; but setup for it is a little verbose; see this comment: #1361 (comment)

I was considering adding a helper function that does all that boilerplate, so you can basically do something like:

const { postgraphile } = require('postgraphile');
const fastify = require('fastify');

const middleware = postgraphile(...);
const app = fastify();
middleware.addToFastify(app);
app.listen(3000);

As you can see from the code in the comment above it's pretty much all boilerplate, so abstracting it should be straightforward. However, doing this wouldn't give quite the same flexibility, especially when it comes to installing route handlers under a subpath, so I'm not really sure if it's the right direction.

@benjie
Copy link
Member

benjie commented Oct 21, 2020

The tests now enable compression middleware on express and fastify3 and all the tests still pass; so this specific issue will be closed when that PR is merged 🙌

@alemagio
Copy link

@benjie
If you don't mind I'll keep my plugin but I'll add a section in my readme with a link to this solution too.

@benjie
Copy link
Member

benjie commented Oct 22, 2020

Of course I don't mind! Writing a Fastify-v3 specific plugin seems out of scope of PostGraphile's server so it definitely makes sense to keep separately, I'm just thinking your plugin could wrap the new methods I'm adding to PostGraphile so it makes it easier for you to maintain 👍

@alemagio
Copy link

That's perfect.
Thanks for all your work ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔁 revisit-in-v5 Not happy with this behaviour, but changing it is a breaking change.
Projects
Ancient "V5" plan
  
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants