Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

jankeromnes
Copy link
Member

@jankeromnes jankeromnes commented Mar 13, 2019

Fixes #2009.

This is #3768, but:

Since I couldn't contribute to #3768, I've created a new pull request, but preserved the Co-authored-by: information.

@kittaakos
Copy link
Contributor

@jankeromnes, I think we should see a successful build on both Travis and AppVeyor with Node.js 10. Then we can revert that commit.

@jankeromnes
Copy link
Member Author

jankeromnes commented Mar 13, 2019

I've confirmed that this works in Gitpod by running:

nvm install 10
npm config set python /usr/bin/python --global
npm config set python /usr/bin/python
npm install -g typescript yarn

(in an attempt to emulate this upgrade to node 10: gitpod-io/workspace-images#70)

and then yarn to verify that it builds

and then started Theia with cd examples/browser && yarn run start ../.. --hostname 0.0.0.0 to confirm that Theia starts up and can be used normally, without suspicious errors in the browser console or in the server logs.

@jankeromnes
Copy link
Member Author

I think we should see a successful build on both Travis and AppVeyor with Node.js 10. Then we can revert that commit.

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.

@kittaakos
Copy link
Contributor

Do I need to do anything in particular to make Travis and AppVeryor try with Node.js 10?

https://github.com/theia-ide/theia/blob/2bd0e8f4e9f8ba84fe9a10085232c444b8b215a8/appveyor.yml#L5

https://github.com/theia-ide/theia/blob/2bd0e8f4e9f8ba84fe9a10085232c444b8b215a8/.travis.yml#L3

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.

@kittaakos
Copy link
Contributor

And the builds are were failing due to this: #3787 Some details are here.

@jankeromnes
Copy link
Member Author

jankeromnes commented Mar 13, 2019

AppVeyor is green! 💚

However Travis failed with:

lerna ERR!   1 failing
lerna ERR! 
lerna ERR!   1) ripgrep-search-in-workspace-server fails gracefully when rg isn't executable:
lerna ERR!      TypeError: Cannot read property 'on' of undefined
lerna ERR!       at RipgrepSearchInWorkspaceServer.search (lib/node/ripgrep-search-in-workspace-server.js:31:79)
lerna ERR!       at lib/node/ripgrep-search-in-workspace-server.slow-spec.js:53:619
lerna ERR!       at new Promise (<anonymous>)
lerna ERR!       at Context.<anonymous> (lib/node/ripgrep-search-in-workspace-server.slow-spec.js:52:4850)
lerna ERR!       at step (lib/node/ripgrep-search-in-workspace-server.slow-spec.js:15:56451)
lerna ERR!       at Object.next (lib/node/ripgrep-search-in-workspace-server.slow-spec.js:15:53580)
lerna ERR!       at lib/node/ripgrep-search-in-workspace-server.slow-spec.js:15:52746
lerna ERR!       at new Promise (<anonymous>)
lerna ERR!       at __awaiter (lib/node/ripgrep-search-in-workspace-server.slow-spec.js:15:51901)
lerna ERR!       at Context.<anonymous> (lib/node/ripgrep-search-in-workspace-server.slow-spec.js:52:4577)

Maybe some API change? The only occurrences of .on I can find in ripgrep-search-in-workspace-server.ts are:

https://github.com/theia-ide/theia/blob/2bd0e8f4e9f8ba84fe9a10085232c444b8b215a8/packages/search-in-workspace/src/node/ripgrep-search-in-workspace-server.ts#L160

and

https://github.com/theia-ide/theia/blob/2bd0e8f4e9f8ba84fe9a10085232c444b8b215a8/packages/search-in-workspace/src/node/ripgrep-search-in-workspace-server.ts#L227

Maybe process.output is undefined for some reason?

@@ -1,6 +1,6 @@
sudo: required
language: node_js
node_js: '8'
node_js: '10'
Copy link
Member

@akosyakov akosyakov Mar 14, 2019

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

Copy link
Member Author

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.

jankeromnes and others added 2 commits March 14, 2019 08:14
Co-authored-by: Alex Tugarev <alex.tugarev@typefox.io>
Co-authored-by: Akos Kitta <kittaakos@typefox.io>
Signed-off-by: Jan Keromnes <jan.keromnes@typefox.io>
…yor.

Signed-off-by: Jan Keromnes <jan.keromnes@typefox.io>
@jankeromnes
Copy link
Member Author

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 catch (error):

https://github.com/theia-ide/theia/blob/2bd0e8f4e9f8ba84fe9a10085232c444b8b215a8/packages/process/src/node/raw-process.ts#L131-L142

But only this when process.on('error'):

https://github.com/theia-ide/theia/blob/2bd0e8f4e9f8ba84fe9a10085232c444b8b215a8/packages/process/src/node/raw-process.ts#L108-L112

(Wouldn't the streams stay undefined in that second case?)

@paul-marechal
Copy link
Member

paul-marechal commented Mar 19, 2019

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

@akosyakov
Copy link
Member

@jankeromnes Would it be fined to close in favour of #4750?

@jankeromnes
Copy link
Member Author

Would it be fined to close in favour of #4750?

Yes please! Thank you @thegecko for addressing this. 👍

@jankeromnes jankeromnes closed this Apr 8, 2019
@jankeromnes jankeromnes deleted the fix-2009 branch April 8, 2019 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants