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

fix: issue #7892 prevent package-lock.json creation without package.json #7960

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

Conversation

Kyle-Ignis
Copy link

fix for npm install creating package-lock.json file when accidentally run in the wrong directory in a project.

References

Fixes #7892

@Kyle-Ignis Kyle-Ignis requested a review from a team as a code owner December 5, 2024 04:52
@Kyle-Ignis
Copy link
Author

@wraithgar let me know how this stab looks. Appreciate the patience.

if (!fs.existsSync(path.join(options.path, 'package.json'))) {
log.error('No package.json found in the current directory.')
log.error('Please navigate to the correct directory or run npm init.')
return Promise.resolve('Aborted due to missing package.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant Promise.reject? and then you'd need to wrap the message in a new Error().

But we could just make this async and throw properly too.

Copy link
Author

Choose a reason for hiding this comment

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

I believe I have resolved this now.

@Kyle-Ignis
Copy link
Author

Ready for review again, thank you for your patience again!

@Kyle-Ignis Kyle-Ignis requested a review from reggi December 5, 2024 21:19
@@ -30,19 +32,23 @@ const printDiff = diff => {
})
}

module.exports = (options, time) => {
module.exports = async (options, time) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, didn't catch that this was a bin, not part of the export. This doesn't actually change arborist itself. All this changes is npx -p @npmcli/arborist reify

Copy link
Author

@Kyle-Ignis Kyle-Ignis Dec 6, 2024

Choose a reason for hiding this comment

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

Can you explain further or point me in the right direction?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's also not a breaking change (as long as the syntax is supported in all supported node's, which i believe it is), because the function already returns a promise.

@Kyle-Ignis
Copy link
Author

Would be really nice to have this extremely annoying bug fixed.

@Kyle-Ignis Kyle-Ignis requested a review from wraithgar December 20, 2024 16:57
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.

[BUG] npm install creates directories and empty package.json
4 participants