jaydenseric / graphql-upload

Middleware and a scalar Upload 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 132 forks source link

Add support for Node 6.x #109

Closed andyrichardson closed 5 years ago

andyrichardson commented 6 years ago

Hi there, first off, thanks for the great project!

The team working on apollo-server are trying to maintain support for Node v6.x (source). According to @evans, updating to the 6.0.0-alpha.1 would break this support.

Are there any plans to maintain compatibility with v6.x LTS releases (which still has 8 months until EOL)?

jaydenseric commented 6 years ago

I have been working full time on an apollo-upload-server v7 release (skipping a stable v6 release) for much of this week, and it's nearly ready. It will be for Node.js 8.5+, like the v6 alpha.

After the release, we can look at what can be done to support Node.js v6 in a potential semver minor update. It would also involve changing the level of support and publishing new versions for fs-capacitor and jsdoc-md. It's not just a matter of transpilation ( 💩regenerator runtime dependencies, etc. that unfairly hit users on decent versions of Node.js); Node.js APIs to do with destroying streams, etc. are different in older Node.js versions. I'm not expecting it will be easy to get all the tests to pass, which have already taken months of effort for the more current versions.

Ideally apollo-server, which is pretty new, would lift their support to Node.js v8.5+. I doubt many people are starting new Node.js projects in late 2018 with Node.js v6 when a vastly superior v10 is out. Node.js v6 doesn't even have async/await.

jaydenseric commented 6 years ago

v7.0.0 is out 🚀

It has a lot of critical improvements and fixes; I think we have finally ironed out the kinks.

The user facing API changes are backwards-compatible (with deprecation warnings, see https://github.com/jaydenseric/apollo-upload-server/pull/107), so other than the Node.js version issue it should be appropriate for an apollo-server@2 minor update.

I'm a week behind my paid work now, so I will have to circle back around to this issue after finishing up the release of the next apollo-upload-client: https://github.com/jaydenseric/apollo-upload-client/blob/master/changelog.md#next. It has also been affected by recent Babel runtime changes.

If you would like to help expedite Node.js v6 support, the best place to start is with a PR to fs-capacitor. @mike-marcacci has tried out Node.js v6 with it, and unsurprisingly issues around destroying streams will need to be resolved.

jaydenseric commented 6 years ago

When it comes to it, here is how the --experimental-modules testing can be skipped for when CI Node.js < v8.5: https://github.com/jaydenseric/extract-files/blob/v4.0.0/package.json#L68

andyrichardson commented 6 years ago

Congrats on the release! I'll have a tinker around in my free time but i'm not sure how helpful I will be with this.

It makes a lot of sense to not support Node v6 but a fix for apollo-server would be super sweet!

abernix commented 6 years ago

While newer versions of Node.js (e.g. 8, 10) have substantial improvements in terms of features and performance, thanks to their rapid updates to the underlying V8 engine, it's not always possible for users to adopt these newer nodes. The apollo-server project itself may be newly revitalized, but many Node.js deployments still subscribe to the LTS support terms which were in place by the Node.js Foundation at the time.

As @andyrichardson mentioned, LTS support is still in place on Node.js 6 for another eight months. It's certainly tempting to consolidate the complexity of version matrixes and test cases involved with supporting multiple Node.js engines, but I think it's important for us as a community to rally behind the Node.js Foundation's LTS schedule (linked above) since it's one of the only published guidelines in the Node.js ecosystem in terms of "what to support and how long?". For example, npm itself subscribes to a pattern where they will support (and continue to back-port to) any npm version which is contained within an maintenance LTS version of Node.js.

The Apollo project doesn't publish LTS terms, but we do generally try to provide support in a similar way as npm does (paired with Node.js LTS). We have a lot of users to consider when dropping support for older runtimes and many have to wait until their infrastructure catches up. For example, (shockingly) Amazon only added Node.js 8 support to Lambda in April 2018. This doesn't mean that we're clinging onto old Node.js versions unnecessarily though; Apollo Server dropped support for Node.js 4 the same week that the Node.js Foundation did — so we're certainly eager to move forward ourselves!

That said, I'll/we'll investigate a solution for apollo-server. @mike-marcacci if you have any details regarding problems you've encountered, I'd be super interested in looking at ways to overcome those! Alternatively, it does seem that including core-js as a (hoisted) dependency within apollo-server might be a short-term option.

jaydenseric commented 6 years ago

Node.js v8 and v10 are out, and are LTS releases. Supporting the last 3 major releases (2 being LTS) is more than reasonable. LTS means if you build something, expect security patches from Node.js to maintain it for a reasonable service life. It doesn't mean every npm package major release for 3 years will support it, or that it's a good idea to start new projects with it in 2.5 years time.

Sometimes, people can't be bothered upgrading Node.js because "if it ain't broke". Once they have a hard reason to upgrade, they do. It's not like browser support where we are slaves to the preferences of the public.

It's only a very recent thing that the latest @babel/runtime v7 RC releases have cut down on the babel-runtime bloat, which was nearly ~8.7 MB on disk!

screen shot 2018-08-20 at 10 20 23 pm

Still, stack traces and things are much nicer with native async/await; I'm not super excited to regress. I use apollo-upload-server in my own projects and as someone who keeps Node.js up to date, want it to be as lean and clean as current tech allows. I bet the majority of apollo-upload-server consumers feel the same way.

abernix commented 6 years ago

stack traces and things are much nicer with native async/await; I'm not super excited to regress

As someone who has seen the debugging experience evolve from node-inspector days into what it is now in Node.js 8+, I completely understand! I didn't intend to downplay the glory of native functionality (like async/await) or that the @babel/runtime changes aren't also important — though it could be argued the savings obtained from that are less interesting in server-only environments (though it was a major surprise to come just before the Babel 7 RC!)

It doesn't mean every npm package major release for 3 years will support it, or that it's a good idea to start new projects with it in 2.5 years time.

Definitely not, but you can imagine someone who has deployed a server 2.5 years ago might want to add GraphQL support to it and could be constrained to Node.js 6 for other reasons.

Node.js v8 and v10 are out, and are LTS releases. Supporting the last 3 major releases (2 being LTS) is more than reasonable.

In Node.js terms, Current and LTS are mutually exclusive. Current does not imply LTS, but rather that, if it is even-numbered, it is intended to become Active LTS. (Node.js 9 was also Current but is odd-numbered and was destined for being dropped). Node.js 10 is still subject to non-trivial breaking changes which the Active LTS is not subjected to and doesn't earn this Active title until October 2018 (and then, only if the Node TSC decides it is ready). In terms of considering a safe choice for a production environment, if you're concerned about breaking changes, it's important to look at the Active LTS and not the Current. Under this versioning/grouping scheme, which serious production users should consider, there are only two LTS versions which are not slated (by definition) to receive potentially breaking changes: Node.js 6 and Node.js 8 — the two versions which apollo-server aims to support. You are only supporting one LTS version of Node.js at this time.

All that said, your desire to go with the latest and the greatest is certainly your prerogative and the presence of what sound like vulnerabilities in v5.0.0 (as you recently mentioned in https://github.com/jaydenseric/apollo-upload-server/issues/103#issuecomment-414312891) seems to be a pressing reason to move away from the current v5.0.0 version — something that we intend to do with apollo-server as practical. Unfortunately, we have pledged to support Node.js 6 and not every one of our users is an early adopter or on the bleeding-edge so it seems we'll need to work on another option to maintain that support.

Thanks for your quick response!

mike-marcacci commented 6 years ago

Hey @abernix! I totally understand where you're coming from here, and I certainly agree that it would be ideal to support the full range of engines supported by The Node Foundation.

However, my primary concern is actually implementing this correctly, given the nature of the problem we're solving here and the preponderance of subtle variations in the APIs across these versions of node. Between node 6 and 10, there are critical differences in the way the http, stream, and fs libraries are implemented that aren't terribly obvious, but have a huge impact on how they work.

For example, the order in which stream events are emitted under several conditions are different between 6, 8, and 10. Getting this library and fs-capacitor (which I actually built for this project, but kept separate since it solves a more generic problem) to work consistently on 8, 9, and 10 was extremely challenging, and required lots of testing and reading the standard library's relevant source code for all concerned versions.

As another example, reliably detecting an aborted http request proved to be surprisingly challenging, since no version of IncomingMessage would emit the documented "aborted" event, and 10 would emit a "close" event immediately, while 8 and 9 wouldn't.

My top priority is the program's soundness. This is fairly trivial when just targeting a single version of node, but is much more challenging while supporting for multiple versions. I am completely open to supporting node 6 here – I would really like that, in fact – but I want to do so in a way that behaves correctly in all scenarios, and doesn't compound the existing complexity.

Because we interface with both the network and the filesystem, there are a lot of opportunities for nondeterministic behavior and race conditions, which have a habit of hiding from tests, only to break in production. To help find nondeterministic behavior and incorrect assumptions, I run the test suite hundreds of times in a bash while loop, until it either hangs or fails (or takes unreasonably long).

It could be the case that 6 and 8 are far more similar to each other than 8 and 10 were, so this might not be as difficult as I described. If anybody wants to take a stab at it, PRs are always welcome!

mike-marcacci commented 6 years ago

As I've had time to think about this a bit more, I realized I could explain the challenge a bit better. The problem is that we depend on parts of the standard library that changed between the different versions of node... it's not that we necessarily need new language features (since we can quite easily transpile our code); rather, the challenge is that we can't specify the versions of the http, stream and fs libraries we use. Instead, we have to find tricks to keep behavior consistent when we could simply require a different version if these were typical npm packages.

jaydenseric commented 6 years ago

I've published jsdoc-md v1.6.0, which shifts Node.js support from v8.5+ to v6+ and bumped the dev dependency here on master branch. The next step is for someone to do an fs-capacitor PR. I'll only update engines and Babel config here once fs-capacitor supports Node.js v6.

abernix commented 6 years ago

Just a heads up: I haven't forgot about this, but haven't had the time to revisit it yet. If I can find some time to dig into fs-capacitor, I definitely will.

All in all, I really appreciate the additional clarity on the struggles you encountered above, @mike-marcacci. 🙇

jaydenseric commented 5 years ago

apollo-server@2.3.0-alpha.0 has updated to graphql-upload@8, dropping file upload support for Node.js < v8.5:

https://github.com/apollographql/apollo-server/pull/2054#issuecomment-444471202

Apollo's earlier requirements were the main reason for this issue to remain open. It is unlikelygraphql-upload will ever support Node.js v6 as it appears to be prohibitively difficult, has downsides for modern Node.js projects, and it's approaching end of life in a few months anyway.

Sorry to anyone who was holding out hope 😅