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(config_file): add config file option npmRegistry #19317

Closed
wants to merge 1 commit into from

Conversation

marcushultman
Copy link
Contributor

@marcushultman marcushultman commented May 30, 2023

Adding Configuration file option "npmRegistry", respected in the LSP and in the runtime. The NPM_CONFIG_REGISTRY env var takes precedence. Both "npmRegistry" and NPM_CONFIG_REGISTRY are treated the same in terms of trailing slash (added if not already present).

Closes #16105

In the issue there are calls for a per-scope configurable value, but perhaps this is a good enough first milestone (since it makes the LSP usable with a custom registry). The config value can also be expanded in the future to be either a string url or an object map from scope to registry url, building on top this behavior.

@CLAassistant
Copy link

CLAassistant commented May 30, 2023

CLA assistant check
All committers have signed the CLA.

cli/lsp/config.rs Outdated Show resolved Hide resolved
@marcushultman marcushultman force-pushed the lsp-registry branch 3 times, most recently from f790b05 to 42efb61 Compare May 30, 2023 17:37
@marcushultman
Copy link
Contributor Author

@dsherret This seems to work pretty well. Adding an option might be a controversial thing though, do you know the process/next steps to have this functionality land?

@marcushultman marcushultman changed the title feat(lsp): support configurable npm registry for npm specifiers feat(config_file): add config file option npmRegistry May 30, 2023
@marcushultman marcushultman force-pushed the lsp-registry branch 2 times, most recently from b7c1032 to ee64caa Compare May 30, 2023 19:56
@marcushultman marcushultman marked this pull request as ready for review May 30, 2023 20:19
@lino-levan
Copy link
Contributor

Wondering if a flag would make more sense rather than a config file option.

@marcushultman
Copy link
Contributor Author

Wondering if a flag would make more sense rather than a config file option.

@lino-levan That wouldn't work for the LSP, or how would you suggest to solve that? I solved that in a previous iteration of this PR where the url was given to the LSP via a setting (so in VS Code you could specify the url).

@lino-levan
Copy link
Contributor

I'm a fan of making it an option in the vscode extension, feels a bit iffy to have yet another top level config option for this. Wonder if it's worth making all of the npm/node stuff under a single option?

@marcushultman
Copy link
Contributor Author

marcushultman commented May 31, 2023

I'm a fan of making it an option in the vscode extension, feels a bit iffy to have yet another top level config option for this. Wonder if it's worth making all of the npm/node stuff under a single option?

I do think it makes sense to use a config option in this case, since it's critical for both the LSP and the runtime. If it was only relevant to one or the other, I can see how a flag (+optional config option) or a LSP setting would be preferable. And as for the syntactics, I'm unopinionated. I just want to make it work for the company-internal npm mirror.

@marcushultman
Copy link
Contributor Author

I did not realize that I could just start the VS Code process and locally override the NPM_CONFIG_REGISTRY env var. So this feature is not strictly required to make things work, but it does ease the usage.

@dsherret
Copy link
Member

I'm a fan of making it an option in the vscode extension, feels a bit iffy to have yet another top level config option for this. Wonder if it's worth making all of the npm/node stuff under a single option?

Yeah, I agree because this is often a user setting. Let's implement denoland/vscode_deno#858 (comment) for now.

@dsherret dsherret closed this Sep 18, 2023
btoo added a commit to btoo/vscode_deno that referenced this pull request Jan 16, 2024
btoo added a commit to btoo/vscode_deno that referenced this pull request Jan 16, 2024
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.

Private registry / Configure npm registry for module download
4 participants