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

feat: ask to install local node runtime if not available in PATH #51

Merged
merged 17 commits into from
Feb 22, 2021

Conversation

rchl
Copy link
Member

@rchl rchl commented Feb 1, 2021

Needs to be tested on Linux. On Mac extracting *.tar.xz fails in Python 3.3 and I needed to switch to *.tar.gz. The same might be necessary for Linux.

On ST3 Node installation happens on the main thread so that's not ideal.

@rchl
Copy link
Member Author

rchl commented Feb 2, 2021

I've tried on Linux and had to switch to downloading *.tar.gz there also as the issue is with the tarfile library in Python 3.3, it seems.


The logic with this change is that at first the Node available on PATH will be tried and if it exists and matches the minimum version then it will be used. Otherwise, we'll install a local Node version in Package Storage. The version is hardcoded to 14.something currently.


Note the Node-based LSP-* packages that define the command in their settings will need to be adjusted to let them use the local version, if available. The command will need to be changed from:

"command": ["node", "${server_path}", "--stdio"],

to

"command": ["${node_bin}", "${server_path}", "--stdio"],

, for example.

Not knowing that detail can confuse you in case you want to test it.

@rchl
Copy link
Member Author

rchl commented Feb 3, 2021

I was also wondering if the current logic is a good default. I mean, trying Node on PATH first and falling back to the local one. Maybe the local one should be always used instead to make things more consistent and predictable.

And maybe there should be a setting to control all that to allow the user to do whatever he/she wants.

@rwols
Copy link
Member

rwols commented Feb 8, 2021

You should have a dialog offering to install this, or have clear docs about this. In any case, installing a runtime in the background, without notice, should be avoided.

@jfcherng
Copy link
Contributor

jfcherng commented Feb 9, 2021

Note that Node 14 will not run on Windows 7. If node 14 must be used, you can skip the platform check via https://nodejs.org/api/cli.html#cli_node_skip_platform_check_value env variable.


Ref: nodejs/node#33000 (comment)

@rchl
Copy link
Member Author

rchl commented Feb 9, 2021

You should have a dialog offering to install this, or have clear docs about this. In any case, installing a runtime in the background, without notice, should be avoided.

I agree. Only that the available options for showing a prompt to a user are not ideal... I guess the best one to use would be yes_no_cancel_dialog but it's awkward to use it.

Note that Node 14 will not run on Windows 7. If node 14 must be used, you can skip the platform check via nodejs.org/api/cli.html#cli_node_skip_platform_check_value env variable.

Ref: nodejs/node#33000 (comment)

In that case, I'll probably switch to Node 12 by default.

@rchl
Copy link
Member Author

rchl commented Feb 15, 2021

I'd say I'm pretty much ready here.

The dialog asking to install a local Node is implemented. One thing that I'm thinking about is to remember user's decision permanently when he says not to install Node (for example using a three-button dialog). Not sure where I would store those though (can dependencies have their own *.sublime-settings?) or how the user would then know how to reset that setting.

For now, the NO answer is remembered for the session and I'm thinking that if user doesn't want to be reminded on every start then he/she should either uninstall the package or fix the Node on PATH.

@predragnikolic
Copy link
Member

Tested on linux it works as described :)

Maybe the local one should be always used instead to make things more consistent and predictable.

I wanted also to suggest something to use the NodeDistributionPATH (because implementation would be simpler)
instead of having checking NodeDistributionLocal and if it exist and satisfies minimal version number... use NodeDistributionLocal, else prompt NodeDistributionPATH installation...

But on the other side, the behavior in this PR is great.
If the user already has node installed the installation is faster.

But on the other side, it takes less than one minute(<40s) to download node.


Is it possible to implement some way of cleanup logic(delete the node dist path, if no longer needed)?

@rchl
Copy link
Member Author

rchl commented Feb 16, 2021

I wanted also to suggest something to use the NodeDistributionPATH (because implementation would be simpler)
instead of having checking NodeDistributionLocal and if it exist and satisfies minimal version number... use NodeDistributionLocal, else prompt NodeDistributionPATH installation...

It's a bit hard to understand what you mean here. :)

Is it possible to implement some way of cleanup logic(delete the node dist path, if no longer needed)?

It should be easy to implement now but could prove difficult if I decide to support installing multiple versions. I'm thinking that it would be worth supporting a behavior where if a package specifies a minimum node version higher than the default one, we would install that one also.

@predragnikolic
Copy link
Member

I wanted also to suggest something to use the NodeDistributionPATH (because implementation would be simpler)
instead of having checking NodeDistributionLocal and if it exist and satisfies minimal version number... use NodeDistributionLocal, else prompt NodeDistributionPATH installation...

It was late and I was tired, hahaha, sorry for that message above :)

Basically my concern was that I thought that the current logic was maybe complex.

  • Check if node is locally installed
  • and if it meets the min node version, install the server with local node,
  • else install node in the Package storage folder and use that node to install the server.

I wanted to suggest to always install node in the Package storage and always use that. (because it takes less than 40s to download node and the implementation will be simpler)

But I am happy with the current state of this PR.

@rchl rchl changed the title feat: install node in package storage if not available in PATH feat: ask to install local node runtime if not available in PATH Feb 22, 2021
@rchl rchl merged commit 9365a69 into master Feb 22, 2021
@rchl rchl deleted the feat/local-node branch February 22, 2021 08:10
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.

4 participants