graphql-upload breaking node hooks

[James-Mnemosyne]

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]

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:

graphql-upload/graphqlUploadExpress.js

Lines 39 to 75 in b1cdd2a

	/**
	* [Express](https://expressjs.com) middleware that processes incoming
	* [GraphQL multipart requests](https://github.com/jaydenseric/graphql-multipart-request-spec)
	* using {@linkcode processRequest}, ignoring non multipart requests. It sets
	* the request `body` to be similar to a conventional GraphQL POST request for
	* following GraphQL middleware to consume.
	* @param {import("express").Request} request
	* @param {import("express").Response} response
	* @param {import("express").NextFunction} next
	*/
	function graphqlUploadExpressMiddleware(request, response, next) {
	if (!request.is("multipart/form-data")) return next();
	const requestEnd = new Promise((resolve) => request.on("end", resolve));
	const { send } = response;
	// @ts-ignore Todo: Find a less hacky way to prevent sending a response
	// before the request has ended.
	response.send =
	/** @param {Array<unknown>} args */
	(...args) => {
	requestEnd.then(() => {
	response.send = send;
	response.send(...args);
	});
	};
	processRequest(request, response, processRequestOptions)
	.then((body) => {
	request.body = body;
	next();
	})
	.catch((error) => {
	if (error.status && error.expose) response.status(error.status);
	next(error);
	});
	}

The most hacky thing about it is this:

graphql-upload/graphqlUploadExpress.js

Lines 55 to 64 in b1cdd2a

	// @ts-ignore Todo: Find a less hacky way to prevent sending a response
	// before the request has ended.
	response.send =
	/** @param {Array<unknown>} args */
	(...args) => {
	requestEnd.then(() => {
	response.send = send;
	response.send(...args);
	});
	};

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

[James-Mnemosyne]

Awesome. Thanks.

[James-Mnemosyne]

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]

@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]

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]

@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.