jaydenseric / graphql-upload

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.43k stars 131 forks source link

graphql-upload breaking node hooks #320

Closed James-Mnemosyne closed 2 years ago

James-Mnemosyne commented 2 years ago

Libraries using node hooks seem to be incapable of using node hooks.

In my case, in graphql middleware, I set an object in the node context and then fetch it:

E.g.

// basically just a namespaced wrapper around node-hooks.
import httpContext from 'express-http-context';

export function setObject(obj) {
  httpContext.set('object', obj);
}

export function getObject() {
  return httpContext.get('object');
}

And in the middleware:

const obj = { ...things };
setObject(obj);

// ...later
const obj = getObject();

This works correctly for every request I have except for uploads.

What makes it stranger is that it only fails for larger uploads. 50kb files upload fine, but 500kb files fail. Both the setObject and getObject are only called once, no matter the size of the upload, and it's within a fraction of a second of each other (in fact, they occur within the same function call), which makes this a bit baffling, because in any case where getObject is called, setObject is guaranteed to have been called.

Is there some aspect of graphql upload that disables node hooks?

jaydenseric commented 2 years ago

Libraries using node hooks seem to be incapable of using react hooks.

That's a confusing statement. How are React hooks relevant?

I don't know what that "node hooks" stuff is about, but it sounds very fragile. It seems the sort of problems you are describing can be expected:

Note that some popular middlewares (such as body-parser, express-jwt) may cause context to get lost. To workaround such issues, you are advised to use any third party middleware that does NOT need the context BEFORE you use this middleware. — https://github.com/skonves/express-http-context/tree/v1.2.4#how-to-use-it

You can see how the graphql-upload Express middleware works:

https://github.com/jaydenseric/graphql-upload/blob/b1cdd2a913c5394b5ff5f89b28d79b949b0bdde5/graphqlUploadExpress.js#L39-L75

The most hacky thing about it is this:

https://github.com/jaydenseric/graphql-upload/blob/b1cdd2a913c5394b5ff5f89b28d79b949b0bdde5/graphqlUploadExpress.js#L55-L64

So I would start there if you have considered everything else.

James-Mnemosyne commented 2 years ago

Awesome. Thanks.

James-Mnemosyne commented 2 years ago

Actually, I was misunderstanding the above. The express middleware is guaranteed to run before the gql middleware, no? I'll take a deeper look, but if it's alright, I'll leave this open for now, so I can put a fix here if I figure out what's going on.

Node hooks docs: https://nodejs.org/api/async_hooks.html

jaydenseric commented 2 years ago

@James-Mnemosyne I'm still not sure what this issue is about. We only keep issues open in this project if it's something that I could action; what is it that you would like me to do to resolve this issue?

James-Mnemosyne commented 2 years ago

Yeah. I haven't had time to dive into it, and probably won't for a bit. Sorry about that, and about the delay.

Will close, but if there's a record of known issues, it might be worth jotting down somewhere that graphql-upload breaks node's hook functionality, even if it's only the high level incompatability that's apparent.

koresar commented 2 years ago

@James-Mnemosyne My fork of graphql-upload uses classic require() only. Should not break hooks. https://github.com/flash-oss/graphql-upload-minimal Hope this helps.