-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
GH-2009: Updated the dependencies to support Node.js 10+. #4556
Conversation
@jankeromnes, I think we should see a successful build on both Travis and AppVeyor with Node.js 10. Then we can revert that commit. |
I've confirmed that this works in Gitpod by running:
(in an attempt to emulate this upgrade to node 10: gitpod-io/workspace-images#70) and then and then started Theia with |
Do I need to do anything in particular to make Travis and AppVeryor try with Node.js 10? I'll also need to test the Electron app locally. |
No. Electron comes with its own Node.js. It is sufficient if the CIs pass. |
AppVeyor is green! 💚 However Travis failed with:
Maybe some API change? The only occurrences of and Maybe |
or by: (or maybe it isn't set at all?) |
@@ -1,6 +1,6 @@ | |||
sudo: required | |||
language: node_js | |||
node_js: '8' | |||
node_js: '10' |
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.
So our base line now nodejs 10? If not it should stay nodejs 8, we should avoid using any nodejs 10 apis
the same for appveyour
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.
It's just a test to verify that the other commit fixes Theia for Node 10. I'll revert it once the tests pass.
Co-authored-by: Alex Tugarev <[email protected]> Co-authored-by: Akos Kitta <[email protected]> Signed-off-by: Jan Keromnes <[email protected]>
…yor. Signed-off-by: Jan Keromnes <[email protected]>
Rebased in order to retrigger the tests, and see if the error reproduces. Also, it looks very suspicious to me that we do this when But only this when (Wouldn't the streams stay undefined in that second case?) |
@jankeromnes in the second case streams wouldn't be undefined, as this is due to the way asynchronous APIs work in NodeJS: when you register callbacks, they will only be called on the next tick (or later), unlike errors thrown and caught synchronously (first case). |
@jankeromnes Would it be fined to close in favour of #4750? |
Fixes #2009.
This is #3768, but:
10.x.x
. #3768 (comment)yarn.lock
where only the 6 relevant dependencies were upgradedSince I couldn't contribute to #3768, I've created a new pull request, but preserved the
Co-authored-by:
information.