Upgrade to graphql-upload v8, dropping upload support for Node.js v6. #2054
Conversation
Due to changes in the third-party `graphql-upload` package which Apollo Server utilizes to implement out-of-the-box file upload functionality, we must drop support for file uploads in versions of the Node.js engine prior to v8.5.0. Since file uploads are supported by default in Apollo Server 2.x, and there is an explicit dependency on `graphql-upload`, we must prevent users who are affected by this mid-major-release deprecation by being surprised by the sudden lack of upload support. By `throw`-ing an error at server startup for affected users, we certainly are breaking a semantic versioning agreement for these users, however with a relatively simple ergonomic (setting `uploads: false`) we allow those users who are NOT utilizing file uploads (as we believe is the case with a majority) to continue using their version of Node.js until it reaches the end of its supported lifetime (as dictated by its Long Term Support agreement with the Node.js Foundation). If we did not `throw` the error at server start-up, those affected may not notice since they may update and start their updated server without noticing the impending chance of failure when someone tries updating! Apollo Server 2.x has attempted to maintain full compatibility with versions of Node.js which are still under Long Term Support agreements with the Node.js Foundation. While this continues to mostly be true, file uploads are an exception which we've now had to make. Third-party open-source projects must absolutely do what's best for their project. From an architecture standpoint, I suspect that we (the designers behind Apollo Server) are mostly to blame for this. Namely, it's unfortunate that we had made such an incredibly coupled integration with a third-party package that we restricted our users from incrementally adopting the changes (and new/improved functionality) of, in this particular case, the `graphql-upload` package. I hope we can take better care with decisions like this in the future! Lastly, this commit also adds documentation to help those affected.
|
Unsurprisingly, tests are failing on Node.js 6. |
abernix
force-pushed
the
abernix/throw-when-uploads-on-pre-node-8.5
branch
from
9dcb571
to
7e6d6cf
Compare
Now that there are specific versions of Node.js which don't support file uploads (namely, <= 8.5.0) we need to explicitly disable uploads on those versions, similar to how those users must opt-in to that behavior by setting `uploads: false` in their Apollo Server constructor options. This effectively accomplishes exactly that, but only when necessary.
I hope to actually remove this limitation, but I'm going to iterate on it first. This also just switches to a `NODE_MAJOR_VERSION` constant from the `apollo-server-integration-testsuite` rather than using the GTE function on the same input since...math.
This commit looks way more complicated than it really is thanks to Prettier trying to be helpful. All I've done is add `NODE_MAJOR_VERSION === 6` as a version NOT to test uploads for, in an effort to fix the failing tests (failing appropriately since Node.js 6 with file uploads throws an error right now and cannot run uploads anymore.).
This commit again looks quite complicated, but's merely an over-complication inflicted by Prettification. Disabling whitespace differences when viewing this commit, the functional change here is that we no longer skip many file upload tests when the (semver) major segment of `process.version` is `10`. I couldn't be happier to get rid of this exception for file upload tests on Node.js 10, which was an unfortunate reality of the non-updated `graphql-upload` module world we previously lived in. Thanks, @jaydenseric, for the newfound (to us!) Node.js upload support!
abernix
force-pushed
the
abernix/throw-when-uploads-on-pre-node-8.5
branch
from
bc975c9
to
705256e
Compare
|
Ok, Node.js 6 tests are mostly re-enabled now, thanks to 2fd56f0. Prior to that commit there were 300/563 failing tests but there are only a couple disabled Node.js 6 upload tests now after (the additional) cd6e575 which excludes those. Most happily, we're now actually testing Node.js 10 (implemented in 705256e) and file uploads – something we weren't doing before and I'm pretty happy to have moved on from that unfortunate exception. |
There was a problem hiding this comment.
Great news! Although a semver major release would make more sense, it's awesome Apollo Server will be on graphql-upload v8 soon 🙌
It appears the Apollo Server tests just check the scalar is populated in the resolvers, but not that the entire file actually uploads correctly? It's a good idea to update the file upload tests to follow the modern graphql-upload approach:
https://github.com/jaydenseric/graphql-upload/blob/v8.0.2/src/test.mjs#L76
You can create a file with specific contents to test were uploaded, without having to use fs:
https://github.com/apollographql/apollo-server/pull/2054/files#diff-0f58f3bb90c5ea8c72ee4005cb44afd4R486
https://github.com/jaydenseric/graphql-upload/blob/v8.0.2/src/test.mjs#L90
The stream property has been deprecated (with a warning, it still works for now). The new API to test is createReadStream():
https://github.com/apollographql/apollo-server/pull/2054/files#diff-0f58f3bb90c5ea8c72ee4005cb44afd4R458
https://github.com/jaydenseric/graphql-upload/blob/v8.0.2/src/test.mjs#L97
If you also want to test the deprecated API, here is how it can be done (without spamming deprecation warnings in your console when running tests):
https://github.com/jaydenseric/graphql-upload/blob/v8.0.2/src/test.mjs#L1454
Once you create a stream, it needs to be either consumed properly, wasted (resume()) or destroyed (destroy()). The relevant events need to be promisified and awaited so that the resolver, and server response, don't finish before the request has completed causing disconnect errors. graphql-upload attempts to deal with these situations, but it's better to test a correct implementation.
Here is how, assuming the uploaded test file is a text file:
https://github.com/jaydenseric/graphql-upload/blob/v8.0.2/src/test.mjs#L95
Here is the promisification of the stream events:
https://github.com/jaydenseric/graphql-upload/blob/v8.0.2/src/test.mjs#L56
You can see snapshots here:
These EPIPE checks are not relevant anymore:
|
Merging into |
|
I've published this as In order to gauge the success of the transition, it would be great to get feedback from any existing users who utilize uploads and are able to try it out! |
|
The |
Apollo Server v2.3+ uses graphql-upload, see apollographql/apollo-server#2054 .
* feat(fastify) Apollo Fastify server integration resolve #626 * feat(fastify) Use createHandler instead of applyMiddleware #626 * feat(fastify) Fix integration test for node 10 #626 * feat(fastify) Update README's with fastify createHandler interface #626 * feat(fastify) Implement the fastify createHandler as a synchronous method #626 * (fastify) Tweaks to re-align with the parallel work in #2054. * (fastify): Use port 9999 rather than 8888 for tests. Because Gatsby. This specific port per integration is pretty brittle to begin with, but it does work. Currently, the fact that it works is facilitated by the fact that most people don't use 5555 (Hapi) and 6666 (Express) for anything. That said, the ever-popular Gatsby uses 8888 by default, so let's use 9999! * (fastify) Remove duplicative assertion in upload initialization. * (fastify) Implement fastify upload middleware * (fastify) Fix linting issues * (fastify) Update package-lock
Previously, in order to only load dependencies which only work on Node.js and fail in non-Node.js environments, we introduced a check which did a string-comparison of `process.release.name` to see if it is was `node`. This was first introduced in 9c563fa as part of #2054, but has gone on to be useful for other purposes as well. Some Node.js forks such as NodeSource's N|Solid, which is a fork of Node.js which follows-up each Node.js release with a custom build that includes additional native addons but is otherwise the same, override this value with their own name. This means that N|Source returns `nsolid`, despite the fact that it is almost entirely the same as Node.js. Luckily, N|Solid leaves the base version of its Node.js in `process.versions.node` (and additionally adds its own `process.versions.nsolid`). By relaxing the string comparison on `process.release.name`, we should still be able to accurately detect the environment we want - which is "Close enough to Node.js!". Fixes #2356
Previously, in order to only load dependencies which only work on Node.js and fail in non-Node.js environments, we introduced a check which did a string-comparison of `process.release.name` to see if it is was `node`. This was first introduced in 9c563fa as part of #2054, but has gone on to be useful for other purposes as well. Some Node.js forks such as NodeSource's N|Solid, which is a fork of Node.js which follows-up each Node.js release with a custom build that includes additional native addons but is otherwise the same, override this value with their own name. This means that N|Source returns `nsolid`, despite the fact that it is almost entirely the same as Node.js. Luckily, N|Solid leaves the base version of its Node.js in `process.versions.node` (and additionally adds its own `process.versions.nsolid`). By relaxing the string comparison on `process.release.name`, we should still be able to accurately detect the environment we want - which is "Close enough to Node.js!". Fixes #2356
…#2324) * lazy load unused lambda packages in core * Style adjustments plus changes to account for other PRs with similar motives. The work in #2054 was designed in a way that, irregardless of the environment, the `graphql-upload` package would only be loaded if uploads were enabled. Therefore, the guard which checks `process.env.AWS_EXECUTION_ENV` should no longer be necessary. Additionally, we don't need to prefix our type-only variables with underscores, as that's not a style that we've otherwise adopted. * The work in this was mostly also implemented by #2305, #2304 and #2054, but the remaining subscriptions-transport-ws and unnecessary util.promisify imports are still super worth addressing. So, thank you!
This PR builds upon #1764, and hopes to supersede it — though it stops short of the full deprecation of Node.js 6 that was offered in that solution. Thank you to @lgaticaq for the work in getting that started since I merely needed to rebase this and manage the merge conflicts as the foundation for the rest of the work. The remaining work was done for clarity and understanding to the users who will lose upload functionality on older Node.js versions because of this (necessary) upgrade.
Apollo Server utilizes @jaydenseric's (:wave:)
graphql-uploadpackage to implement out-of-the-box file upload functionality. Changes and improvements to that package which fix potential crashes during file uploads necessitate dropping support for file uploads in versions of the Node.js engine prior to v8.5.0 since the solution revolves around newer APIs which aren't available in older versions of Node.js. There are great improvements and better use of modern APIs which improve upload functionality, so it's certainly worth the upgrade in other regards as well.We'd normally try to abide by more traditional semantic versioning patterns but the solution here ultimately involved dropping support for an Node.js Long Term Support (LTS) version — v6.x — that is still supported by the Node.js Foundation until April 2019. This change brings a partial deprecation for those Node.js v6 users users if they're utilizing uploads. It's worth noting that Node.js 8 did not become "LTS" until v8.9.0, and we aren't trying to support 8.0.0 through 8.5.0 since those are already end-of-life'd.
In an ideal world we could bump to 3.x but, realistically, there are other considerations for us when doing a major version bump — including maintaining backports and updating/forking documentation and educational content (much of which has just been published for 2.x!) — something we're not fully equipped to do at the moment. And ultimately, there is just no solution for Node.js 6 who we still believe should update if they're using uploads. Therefore, we've decided to
throwif certain conditions are met on Node.js < 8.5 (for awareness!) and release this as a semver minor version.Why the
throw? If we did not throw an error at server start-up, those affected may miss it. Since file uploads are supported by default in Apollo Server 2.x, and there is an explicit dependency ongraphql-upload, we must prevent users who are affected by this mid-major-release deprecation by being surprised by the sudden lack of upload support and give users a way to confirm that they don't need uploads since we can't detect it automatically. (I'm happy to ideas here, but I'm afraid we can't know with 100% certainty, which is why we've landed here.) Therefore:uploads: false.By
throw-ing an error at server startup for affected users, we certainly are breaking a semantic versioning agreement for these users, but with good intention and a hopefully resounding reason. Luckily, with a relatively simple ergonomic (settinguploads: false) we allow those users who are NOT utilizing file uploads (as we believe is the case with a majority) to continue using their version of Node.js until it reaches the end of its supported lifetime (again, as per the appropriate timelines).Apollo Server 2.x has attempted to maintain full compatibility with versions of Node.js which are still under LTS agreements with the Node.js Foundation. While this continues to mostly be true, this is an exception which seems to still make sense.
While Node.js v6 is still under Long Term Support from the Node.js Foundation, we encourage all users of Apollo Server to update to newer LTS versions of Node.js prior to the "end-of-life" dates for their current server version.
For example, while Node.js 6.x is covered by Long Term Support until April 2019, there are substantial performance (e.g. V8 improvements) and language changes (e.g. "modern" ECMAScript support) offered by newer Node.js engines (e.g. 8.x, 10.x). Switching to newer Long Term Support versions of Node.js comes with long-term benefits; Node.js 10.x LTS releases are covered by the Node.js Foundation through April 2021!
We intend to drop support for Node.js 6.x in the next major version of Apollo Server.
Lastly, this PR also adds documentation to help those affected — that documentation won't publish until it's merged (and the bit.ly link will 404 until it's published), but it will show on the Netlify preview of this PR for now.
Closes #1854
Closes #1509
Closes #1703
Closes #1764 (Supersedes)