-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade graphql-upload to support Node.js v14 #4039
Conversation
@Leko: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
Thanks for opening this! Are there any other breaking changes to the exposed API (e.g. the In other words, is this okay to introduce in a 2.x release? |
Hi @abernix!
I think It's OK for me but not for all end users. Especially, end users need to migrate to |
It does make sense, but it sounds like we'd still need to put this in a major release to me. Thanks for the clarity. |
Would this be in 3.x, then? |
Note that |
Updating will also solve this issue/bug #3508 with fs-capacitor. In our case we can remove the forced resolution of version 5.0.0 in our package.json files. |
@@ -41,7 +41,7 @@ | |||
"graphql-extensions": "file:../graphql-extensions", | |||
"graphql-tag": "^2.9.2", | |||
"graphql-tools": "^4.0.0", | |||
"graphql-upload": "^8.0.2", | |||
"graphql-upload": "^10.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this to version 11.0.0 as noted by @jaydenseric.
One of the changes in version 11.0.0 is that graphql-upload is actaually CI tested for node version 14.
Version 10.0.0 of graphql-upload is not CI tested for node version 14. See release notes here: https://github.com/jaydenseric/graphql-upload/releases
If I'm not wrong, the next major release is only 30% complete and doesn't have a date. This fix is pretty crucial and should be released sooner is better, because for now |
@abernix Anything you can do to push this a little forward ? |
@Leko could you update your PR to change grapqhl-upload version to 11 ? thx ! |
@abenhamdine I'm sorry for my late reply. I've updated graphql-upload to 11.x. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Did this get handled elsewhere? |
I don't think, see #4304 |
Any updates on when this might be merged? |
Same question ;) |
To those that are inquiring, I posted a suggestion / update on the issue here: #3508 (comment) |
For those that cannot wait and are receiving
|
Couldn't we do a major release with this and rebrand the other upcoming changes to v4? |
after upgrade fs-capacitor in apollo-server-core/node_modules/fs-capacitor to version 6.2.0 I have resolved. |
On node 15, npm v7 even gives a warning when installing |
Is there any reason for not going ahead with this PR? |
@simontaisne I think that the point form @abernix is to put it in the next major release because of the breaking changes. |
As described in #3508 (comment) we will be removing the default graphql-upload integration in Apollo Server 3, so we won't be upgrading it. @abernix has commented above with instructions on how to use graphql-upload directly in Apollo Server 2. |
Hi there!
I've updated
graphql-upload
to v10 to support Node.js v14. The documentation(docs/source/data/file-uploads.md) has already been updated to callcreateReadStream
, but test codes ware still accessing the stream property, so I've updated the test codes.fix #3508 and fix #3579