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: prefix in .npmrc error log #6685

Merged
merged 6 commits into from
Jul 31, 2023
Merged

Conversation

rahulio96
Copy link
Contributor

Summary

The original issue (#6081) wanted the project's .npmrc to set a prefix value for npm to use in both npm prefix and npm install <package> However, we made the changes according to wraithgar's comment.

Currently, when running npm prefix or npm install, there is no error logged communicating to the user that prefix cannot be changed in the project config.

This solution gives the error message more clarity to make debugging easier and lets the user know that prefix cannot be changed from the project level.

Screenshot

Also added the accompanying tap test to ensure the expected error is outputted.

PLEASE NOTE:

If prefix is set to a value, the error will be logged anytime a user enters in a npm or npx command, which means that the logging is not specific to just npm prefix or npm install <package>, but also applies to commands such as npm exec.

Testing

Reproduce Original Issue:

  1. Add .npmrc file into your project’s directory
  2. Set prefix to some location e.g. prefix=./vendor
  3. Run npm prefix or npm install <package>
  4. npm prefix will give the current directory’s prefix, and npm install <package> will install the package in the current directory.
  5. No error would be logged to tell user that prefix cannot be changed from project level

File tap test:

  • Enter in terminal:
  • TAP_SNAPSHOT=1 node . run test workspaces/config/test/index.js

Run the entire tap test:

  • Enter in terminal:
  • node . run test

Dummy Project Test:

  • Made a dummy project with a vendor folder and a .npmrc file with prefix=./vendor
  • Used aliasing to see if the error is being logged:
  • Enter in terminal:
  • alias localnpm="node /workspaces/cli/"
  • localnpm prefix
  • localnpm install <package>

References

Fixes #6081

@rahulio96 rahulio96 requested a review from a team as a code owner July 28, 2023 23:03
Comment on lines 615 to 623
if (type === 'project') {
const parsedConfigPrefix = parsedConfig.prefix
// Log error if prefix is mentioned in project .npmrc
if (parsedConfigPrefix) {
log.error(`prefix=${parsedConfigPrefix} cannot be changed from project config: ${file}`)
}
}
return this.#loadObject(parsedConfig, type, file)
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (type === 'project') {
const parsedConfigPrefix = parsedConfig.prefix
// Log error if prefix is mentioned in project .npmrc
if (parsedConfigPrefix) {
log.error(`prefix=${parsedConfigPrefix} cannot be changed from project config: ${file}`)
}
}
return this.#loadObject(parsedConfig, type, file)
},
if (type === 'project' && parsedConfig.prefix) {
// Log error if prefix is mentioned in project .npmrc
log.error('config', `prefix=${parsedConfigPrefix} cannot be changed from project config: ${file}.`)
}
return this.#loadObject(parsedConfig, type, file)
},

We don't need to cast a whole new variable for one if statement, just assess it in place.

Copy link
Member

Choose a reason for hiding this comment

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

Also added the logging prefix to the log output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, parsedConfigPrefix is also used for the logged error, would you still prefer if we got rid of it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's one lest instance where we're logging user input which is always a good thing to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

I had originally removed it and added it back at the last second, I should have trusted my first instinct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, are you suggesting that we change the log error to be:
log.error('config', `prefix cannot be changed from project config: ${file}.`)

Or are you suggesting this?
log.error('config', `prefix=${parsedConfig.prefix} cannot be changed from project config: ${file}.`)

Copy link
Member

Choose a reason for hiding this comment

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

log.error('config', `prefix cannot be changed from project config: ${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.

Got it, appreciate the response.

@wraithgar
Copy link
Member

Adding config to that logging statement changed the shape of the array in the test. Update the test to expect the new format and this should be good to ship

@rahulio96
Copy link
Contributor Author

Yeah just noticed that too, thanks!

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

This is very helpful thank you.

@rahulio96
Copy link
Contributor Author

No problem, hope it helps.

@wraithgar wraithgar merged commit ed9a461 into npm:latest Jul 31, 2023
22 checks 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.

[BUG] Prefix in .npmrc ignored
3 participants