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

Use npm config when generating the npm lockfile #108

Merged
merged 3 commits into from
Oct 23, 2024
Merged

Conversation

laurence79
Copy link
Contributor

Great tool!

I've just started to use it within a corporate environment and found that when using npm as the package manager, the generate lock file stage can fail if the environment isn't using the default https://registry.npmjs.org repo.

This PR addresses that by loading the npm config, using @npmcli/config and passing that to Arborist.

I've validated the approach using patch-package in the environment in question.

Copy link
Owner

@0x80 0x80 left a comment

Choose a reason for hiding this comment

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

Thanks @laurence79 this looks great! If you could fix these minor things I'll try to publish it soon.

import defaults from "@npmcli/config/lib/definitions/index.js";

export async function loadNpmConfig({ npmPath }: { npmPath: string }) {
const conf = new Config({
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick; I don't think saving these 2 characters is worth making things less readable. I typically avoid abbreviations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, addressed.

.DS_Store Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like I forgot to add this to .gitignore. Could you please add it and remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly embarrassed that this one got committed 😳
Removed and .gitignore updated.

@laurence79
Copy link
Contributor Author

@0x80 comments addressed, cheers!

@0x80 0x80 merged commit 5c703d2 into 0x80:main Oct 23, 2024
1 check passed
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.

2 participants