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

Allow custom npmconfig to be defined in settings.json #9422

Open
1 task done
tacocats opened this issue Mar 15, 2024 · 11 comments · Fixed by #11852
Open
1 task done

Allow custom npmconfig to be defined in settings.json #9422

tacocats opened this issue Mar 15, 2024 · 11 comments · Fixed by #11852
Labels
enhancement [core label] setting Feedback for preferences, configuration, etc

Comments

@tacocats
Copy link

tacocats commented Mar 15, 2024

Check for existing issues

  • Completed

Describe the feature

Issue:

When trying to install a language server or anything that utilizes Node, it uses a default blank config for the --userconfig and --globalconfig. This leads to it pulling its packages from registry.npmjs.org by default, without an option to specify a different location to pull node packages from.

When behind proxies or at some work places, that registry may be blocked or you may want to pull packages from another source.

Proposal:

Since it looks like Zed bundles a node runtime to avoid issues and different node versions may return unexpected results, I don't think having a setting to specify the node path would be needed. But it would be great if we add an option in the user settings to specify the configs to use.

Something like this being added to settings would work:

{
      "node_runtime": {
          "custom_node_user_config": "path/to/config/here",
          "custom_node_global_config": "path/to/config/here"
  }
}

Possible solution:

I did a bit of digging into the issue and it looks like a user specified path could be added here, where it will default to the blank file by default:

Zed calls node with the blankconfigs here:

command.args([
"--userconfig".into(),
installation_path.join("blank_user_npmrc"),
]);
command.args([
"--globalconfig".into(),
installation_path.join("blank_global_npmrc"),

As a temporary workaround I tried modifying the blank configs with a proper config but it looks like each time a command is run it will re-write the default blank config files:

.args(["--userconfig".into(), node_dir.join("blank_user_npmrc")])
.args(["--globalconfig".into(), node_dir.join("blank_global_npmrc")])
.status()
.await;
let valid = matches!(result, Ok(status) if status.success());
if !valid {
_ = fs::remove_dir_all(&node_containing_dir).await;
fs::create_dir(&node_containing_dir)
.await
.context("error creating node containing dir")?;
let file_name = format!("node-{VERSION}-{os}-{arch}.tar.gz");
let url = format!("https://nodejs.org/dist/{VERSION}/{file_name}");
let mut response = self
.http
.get(&url, Default::default(), true)
.await
.context("error downloading Node binary tarball")?;
let decompressed_bytes = GzipDecoder::new(BufReader::new(response.body_mut()));
let archive = Archive::new(decompressed_bytes);
archive.unpack(&node_containing_dir).await?;
}
// Note: Not in the `if !valid {}` so we can populate these for existing installations
_ = fs::create_dir(node_dir.join("cache")).await;
_ = fs::write(node_dir.join("blank_user_npmrc"), []).await;
_ = fs::write(node_dir.join("blank_global_npmrc"), []).await;

I think this would be pretty simple to add and tried taking a look at it as well, but I have no prior rust experience. I did take a look at maybe adding something like this to the node_runtime.js file.

use std::str;
use serde::Deserialize;
use settings::Settings;

#[derive(Deserialize, Clone)]
pub struct NodeSettings {
    pub custom_node_user_config: Option<String>,
    pub custom_node_user_config: Option<String>,
    pub custom_node_global_config: Option<String>,
}

impl Settings for NodeSettings{
    const KEY: Option<&'static str> = Some("message_editor");
    type FileContent = NodeSettings;
    fn load(
        default_value: &Self::FileContent,
        user_values: &[&Self::FileContent],

        _: &mut gpui::AppContext,
    ) -> anyhow::Result<Self> {
        Self::load_via_json_merge(default_value, user_values)
    }
}

If applicable, add mockups / screenshots to help present your vision of the feature

No response

@tacocats tacocats added admin read Pending admin review enhancement [core label] triage Maintainer needs to classify the issue labels Mar 15, 2024
@tacocats
Copy link
Author

Just a side note, I'd be glad to work this if someone could point me in the right direction for how to add user defined values to the settings.json.

@Moshyfawn Moshyfawn added setting Feedback for preferences, configuration, etc and removed triage Maintainer needs to classify the issue labels Mar 19, 2024
@vato-loco
Copy link

🙏

@JosephTLyons JosephTLyons removed the admin read Pending admin review label Mar 26, 2024
@stevetoro
Copy link

This is the main issue stopping me from using zed as my daily driver at work. Any time I edit some JSON or YAML I get an error saying zed couldn't download the language server. Tried to set my job's proxy settings on various npmrc files, but no dice.

osiewicz pushed a commit to RemcoSmitsDev/zed that referenced this issue May 18, 2024
Adding `proxy` keyword to configure proxy while using zed. After setting
the proxy, restart Zed to acctually use the proxy.

Example setting: 
```rust
"proxy" = "socks5://localhost:10808"
"proxy" = "http://127.0.0.1:10809"
```

Closes zed-industries#9424, closes zed-industries#9422, closes zed-industries#8650, closes zed-industries#5032, closes zed-industries#6701,
closes zed-industries#11890

Release Notes:

- Added settings to configure proxy in Zed

---------

Co-authored-by: Jason Lee <huacnlee@gmail.com>
@sa1
Copy link

sa1 commented Jun 25, 2024

Proxy settings is not enough to close this issue. There are other settings like registry that need to be set via custom npmconfig.

@ArielLahiany
Copy link

This is an issue at my workplace as well. I've tried to manually override the NPM configuration file of the NodeJS run-time with no luck.
Would love a suggestion as this currently blocking me from moving to Zed as my daily IDE.

@sa1
Copy link

sa1 commented Jul 25, 2024

@SomeoneToIgnore can you reopen this issue?

@sa1
Copy link

sa1 commented Jul 25, 2024

stderr: "npm ERR! code UNABLE_TO_GET_ISSUER_CERT_LOCALLY\nnpm ERR! errno UNABLE_TO_GET_ISSUER_CERT_LOCALLY\nnpm ERR! request to https://registry.npmjs.org/prettier failed, reason: unable to get local issuer certificate\n\nnpm ERR! A complete log of this run can be found in:\nnpm ERR!     /Users/viu984/Library/Application Support/Zed/node/node-v18.15.0-darwin-arm64/cache/_logs/2024-07-25T14_39_08_962Z-debug-0.log\n"

@tobiaseichert
Copy link

When using intermediary certificates (e.g. those provided by Zscaler), node related downloads triggered by Zed are going to fail due to the fact that npm has to be configured properly as described here:

https://help.zscaler.com/zia/adding-custom-certificate-application-specific-trust-store#npm

An option to point Zed to the proper npm configuration to use would be much appreciated.

@n3oney
Copy link

n3oney commented Aug 15, 2024

Being able to specify a Node path would be very useful for NixOS which can't run unpatched binaries, resulting in errors. So far I've resorted to putting patched Node in the .local/share where zed expects it and it seems to work, but the proper way of being able to add a path to the config file would be much better

@sa1
Copy link

sa1 commented Aug 15, 2024

@n3oney #15739 fixes that particular issue.

@avi-cenna
Copy link
Contributor

@tobiaseichert @sa1 @ArielLahiany please see my comment here, I think I found a workaround.

#4350 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement [core label] setting Feedback for preferences, configuration, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.