File upload plugin

[artecoop]

File upload plugin

[artecoop]

@bmullan91 working on it.
The code is now a bit of a mess, and for debugging purposes I've copied files from grapqhl-upload package what needs to be imported (as dependencies should work lmao)

[bmullan]

Youu sent is to the wrong bmullan The person you want is "bmullan91" not me "bmullan"

…

On Thu, Feb 20, 2020, 5:40 AM Art & Coop ***@***.***> wrote: @bmullan <https://github.com/bmullan> working on it. The code is now a bit of a mess, and for debugging purposes I've copied files from grapqhl-upload package that needs to be imported (as dependencies should work lmao) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#126>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM23J5AOQIK4KLXVRSPIG3RDZMZJANCNFSM4KXW7PDA> .

[bmullan91]

@artecoop I've pushed up that sandbox into an actual repo: https://github.com/bmullan91/fastify-gql-upload-eg

[bmullan91]

@artecoop https://www.npmjs.com/package/fastify-gql-upload - still need to write tests etc, but this should unblock you 😄

[artecoop]

Brilliant work @PabloSzx !!!

[mcollina]

Maybe we can link the plugin from this project README as well?

[mcollina]

You should be able to access that in your resolver as context.reply.request.body.

[artecoop]

Ciao Matteo,
thanks for your reply.

The problem is the request never hit the resolver. the error
Must provide Source. Received: undefined
happens earlier in the fastify pipeline.

[artecoop]

Well, I did it.
Thanks to the debugger and the ability to step upward during debug, I understood my mistakes.
Body was not compliant with fastifyGraphQl function in order to be parsed correctly.
I'll do some refinements now and then I'd love to share my work with you.
@mcollina Do you prefer that I send the code to you or I publish the plug in on my own?

[bmullan91]

@artecoop Nice one! Do you have a branch somewhere where I can look? I'm also digging into this issue :)

[artecoop]

Yeah sorry for that!

[bmullan91]

@artecoop After a lot of head scratching and try and error... I got it working with this before registering the fastify-gql plugin:

 fastify.addContentTypeParser("multipart", (req, done) => {
    req.isMultipart = true;
    done();
  });

  fastify.addHook("preValidation", async function(request, reply) {
    if (!request.req.isMultipart) {
      return;
    }

    // processRequest from `graphql-upload`
    request.body = await processRequest(request.req, reply.res, options);
  });

Here's a codesandbox with the code https://codesandbox.io/s/fastify-gql-upload-eg-7gsz9 - note It fails to write the file on sandbox, not sure if that's possible on codesandbox.

When I moved the code into a separate plugin it no longer worked - not sure why tbh 😕

[artecoop]

@bmullan91 how could this work? graphql-upload's processRequest uses, for example, request.once and response.once that aren't available in fastify's request/response.

I'm prepping a repo with my code to share with all of you.

[bmullan91]

@artecoop Im passing the raw req, and resp via:

await processRequest(request.req, reply.res, options);

[artecoop]

@bmullan91 this is beatiful. Please publish it as npm package. It's way better than my implementation that rewrite the body.

[bmullan91]

@artecoop Will do, once I figure out why fastify doesn't like it when I put it inside its own plugin 😅

[artecoop]

@bmullan91 check this out:
https://github.com/kennyrulez/fastify-gql-plugin
this is my implementation, maybe will help you

[artecoop]

Thank you, it works like a charm

[PabloSzx]

Made a fork of fastify-gql-upload with updated syntax for Fastify v3, updated dependencies + TypeScript support.

https://www.npmjs.com/package/fastify-gql-upload-ts

https://github.com/PabloSzx/fastify-gql-upload-ts

[asciidisco]

Could this be something that should be documented in the README, with a reference to @PabloSzx npm package?
Once documented & accessible to everyone, we should be able to close this, I believe.

[mcollina]

Should we support this natively?

[PabloSzx]

Apollo Server has out of the box support for File Uploads using graphql-upload, I think it would be nice to support it too, and it's not much code needed anyways, but there are some gotchas, like node version compatibility.

[artecoop]

Should we support this natively?

IMHO, no. Keep going with the single responsibility principle, and let other packages, yours or made by other developers, plugs extra features.
Dont waste the "pluggable" effort you did with fastify

[asciidisco]

Should we support this natively?

Might be a good idea to do a quick Pro/Con list (please complete it, if I miss something important)

Pro

• Be compatible with apollo-server, which supports this out of the box (https://www.apollographql.com/docs/apollo-server/data/file-uploads/)
• Necessary changes are minimal, judging from https://github.com/PabloSzx/fastify-gql-upload-ts/blob/master/index.ts

Neutral

• Might have a (minimal) performance impact due to the fact that each request needs to pass an async preValidation hook, which could be mitigated using a config option to disable the feature in case it's not needed: https://github.com/PabloSzx/fastify-gql-upload-ts/blob/master/index.ts#L22

Con

• Additional 3rd party dependency graphql-upload, which itself has 5 more first level dependencies adding quite a bit of weight
• As per @artecoop definition, it violates the single responsibility principle (I'm personally undecided on this fact)
• Has been/Can be solved by a 3rd party module already

My subjective opinion, leave it out of core, but a note should be added to the README with a link to the npm module by @PabloSzx & a quick integration/usage example.

[smolinari]

My vote goes to not adding it in the core and leaving it as a plugin (and yes, I'm new to fastify-gql and my 2 cents probably isn't worth much). 😊

Thing is, how one handles uploading files with a GraphQL server can differ depending on needs and nailing down one way into the core is sub-optimal for everyone else not needing that method. The upload feature is then just bloat. Using plugins means a developer makes the conscious decision to add that feature, and at that point it isn't bloat.

Scott

[mcollina]

closing, I think we have some sort of conclusion :).

[PabloSzx]

btw, the package was renamed to mercurius-upload following the rebranding

https://github.com/PabloSzx/mercurius-upload

https://www.npmjs.com/package/mercurius-upload

[mcollina]

@PabloSzx would you like to move it inside the org to give it an official blessing?

[PabloSzx]

@PabloSzx would you like to move it inside the org to give it an official blessing?

Sure 😁 I added you (matteo.collina) as a mantainer in the npm package, but when I try to "Transfer ownership" of the GitHub repository, this error comes up:

I just added you as a collaborator of I transferred the repo to you and maybe then you can transfer it to the org 👌 .

[mcollina]

Could you add me as an owner on npm as well? I am https://www.npmjs.com/~matteo.collina

[PabloSzx]

Could you add me as an owner on npm as well? I am https://www.npmjs.com/~matteo.collina

Done 👌

[mcollina]

Great!!!!