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: add support for specifying path to node in settings #118

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

taylorzane
Copy link

@taylorzane taylorzane commented Apr 28, 2024

This pull request adds support for specifying a path to a node binary. I've found that system can be unreliable when using tools like nodenv (even with proper .zprofile setup), so this change lets a user specify any node binary to be used with LSP plugins.

I've also submitted a PR to update the lsp_utils.sublime-settings: sublimelsp/LSP#2464

Caveat: This change assumes that an npm binary exists in the same directory as node; in my research it seems that all installations of Node.js follow this principle, so it should be a non-issue.

@predragnikolic
Copy link
Member

Hello,
Thanks for this PR.

I use nvm
and I am on a Mac.

When I have the following set in lsp_utils.sublime-settings:

{
	"nodejs_runtime": ["/Users/predrag/.nvm/versions/node/v20.11.1/bin/node"],
}

I get this error:

Unable to start subprocess for LSP-json
Traceback (most recent call last):
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/server_npm_resource.py", line 123, in install_or_update
    self._node_runtime.run_install(cwd=self._server_dest)
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/node_runtime.py", line 240, in run_install
    raise Exception('Failed to run npm command "{}":\n{}'.format(' '.join(args), error))
Exception: Failed to run npm command "ci --omit=dev --verbose":
env: node: No such file or directory

@taylorzane
Copy link
Author

@predragnikolic I went ahead and made the suggested changes to support nvm. I tested it locally and everything seemed to work fine. Let me know if you're still experiencing runtime errors.

@gerardroche
Copy link
Contributor

gerardroche commented Aug 16, 2024

Why not just add the path to node? Instead of:

"/Users/predrag/.nvm/versions/node/v20.11.1/bin/node"

Allow setting the path as:

"/Users/predrag/.nvm/versions/node/v20.11.1/bin"

Also, filter the path for env variables and user placeholders so that paths like ~/.nvm/versions/node/v20.11.1/bin can be accepted too, use something like:

def filter_path(path):
    """Return a path with user and variables expanded."""
    path = os.path.expanduser(path)
    path = os.path.expandvars(path)

    return path

So now you will be able to set paths like:

~/.nvm/versions/node/v20.11.1/bin
$HOME/.nvm/versions/node/v20.11.1/bin

I'm wondering if it's possible to add the path the environment PATH? It looks like you can do it on a client-by-client basis: see env setting in https://lsp.sublimetext.io/client_configuration/

But you don't want to have to do that for every client, you want to be able to set it globally. That I think would be a better solution:

  • add the path to your node version to env PATH for all clients
  • add the path to your node version to env PATH on a client-by-client basic (looks like you can currently already do this)

Currently, I have to make nvm available at the system level. It's not ideal.

Maybe Sublime Text already has a way of adding to the environment? That wouldn't be ideal either, but it would be better than making nvm available at the system level.

Comment on lines +247 to +248
self._node = path.join(self._base_dir, 'node')
self._npm = path.join(self._base_dir, 'npm')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't work on Windows.

@An-dz
Copy link

An-dz commented Sep 9, 2024

I think this is a way cleaner approach:

 class NodeRuntimePATH(NodeRuntime):
     def __init__(self) -> None:
         super().__init__()
-        self._node = shutil.which('node')
-        self._npm = shutil.which('npm')
+        path = settings.get('nodejs_path') or None
+        self._node = shutil.which('node', path=path)
+        self._npm = shutil.which('npm', path=path)

class NodeRuntimePATH(NodeRuntime):
def __init__(self) -> None:
super().__init__()
self._node = shutil.which('node')
self._npm = shutil.which('npm')

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.

5 participants