Fix #4628: npm install to a wrong location

[geeee]

By default npm install will walk up the folder tree checking for a folder that contains either a package.json file, or a node_modules folder. If such a thing is found, then that is treated as the effective "current directory" for the purpose of running npm commands.
This caused npm dependencies for language servers sometimes to be installed in the wrong folder, described in #4628.
Adding --prefix ./ to npm install forces node_runtime.npm_install_packages to install packages in provided path even if node_modules dir exists somewhere up the filesystem tree from installation path.

Release Notes:

• Fixed an issue where Node based language servers could install in the wrong directory on some systems.

[cla-bot]

We require contributors to sign our Contributor License Agreement, and we don't have @geeee on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

[geeee]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[SomeoneToIgnore]

Nice find.

Two notes:

• It seems safer to have a prefix as a non-relative path. We have installation_path after installing node, where we put the cache, seems that we need to create another directory for the packages there?
• I know that npm uses a cache for resolving remove packages even during the info call, should we use --prefix in that and all other commands, for safety?

If so, run_npm_subcommand could get that new code and make it applicable for all future commands, how do you thing about that?

[geeee]

Thanks for lookin into it!

It seems safer to have a prefix as a non-relative path.

Considering that running npm_install_packages resolves to something like cd ${path} && npm install ... I don't think it matters if --prefix path is relative or not. My thinking was that ./ is easier to debug in this case since the intention is clearer and having cd ... && before npm install kind of guarantees that it's going to be the same path.
Am I missing something? Potential cross platform issues?

We have installation_path after installing node, where we put the cache, seems that we need to create another directory for the packages there?

Isn't this what 'languages' folder is for? Or are you talking about something like root npm package with single node_modules folder used for all languages? If so then this sounds like a design decision that is beyond my competence in this project, obviously. Right now the overlap of packages used in different languages is quite small, so I'm not sure if potential gains are worth it.

I know that npm uses a cache for resolving remove packages even during the info call, should we use --prefix in that and all other commands, for safety?

I'm not sure if it's going to change anything. But if so, it might be a good idea to put this prefix into global_npmrc file and use that file instead of blank_global_npmrc.

[SomeoneToIgnore]

re: relative path

My main idea was to use a more explicit path form to see it in the node logs somewhere, if only the node command fails: otherwise, the real abs path is only used in the Rust code and until adding more logging there we cannot be sure ./ got expanded into something we wanted.
But I would not insist on that, since indeed we're supposed to have the right invocation directory set anyway.

We do indeed install various languages' packages separately, sorry for the confusion, so I guess my cache-related question is not really valid, please disregard it.

I would merge it but let's ask @ForLoveOfCats to have a peek at it before, just in case (my frontend infra knowledge is bad).

[ForLoveOfCats]

This is awesome, we were theorizing that something was causing NPM to install files into the wrong directory and our theory was that it had something to do with the global user config, hence that override, but this makes so much more sense!

I think the relative path is fine, an absolute path would definitely be a bit better but I don't think it matters that much as this patch stands.

I'm much more interested in if we can move this argument into the run_npm_subcommand method so all codepaths interacting with NPM are sure to use the correct path. That would need to use an absolute path, ie the directory argument if present, omitting the argument otherwise.

Thanks so much for figuring this out, we've been scratching our heads over this for quite a while!

[ForLoveOfCats]

🎉 🚢