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(refactor): create new error output primitives #7515

Merged
merged 2 commits into from
May 13, 2024

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented May 13, 2024

These will be used to generate normal and json error messages in the same format from both commands and the exit handler.

This also does a few others things:

  • makes did-you-mean take a package so it can be sync and called more easily from the error handlers
  • standardize all error messages with 2 space indentation to match the rest of npm

@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented May 13, 2024

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only modules-only no-lock no-cache no-modules no-clean show-version run-script cache-only
peer-deps
no-clean
audit
npm@latest 30.291 ±0.50 10.688 ±0.04 11.786 ±0.04 1.585 ±0.02 1.561 ±0.01 1.294 ±0.01 8.280 ±0.01 1.315 ±0.02 0.139 ±0.00 0.166 ±0.00 13.501 ±0.06 3.679 ±2.23
#7515 32.727 ±0.88 10.693 ±0.03 11.727 ±0.04 1.569 ±0.02 1.548 ±0.01 1.266 ±0.00 8.218 ±0.02 1.283 ±0.00 0.140 ±0.00 0.167 ±0.00 14.762 ±0.16 2.153 ±0.01
app-medium clean lock-only cache-only modules-only no-lock no-cache no-modules no-clean show-version run-script cache-only
peer-deps
no-clean
audit
npm@latest 23.548 ±0.00 7.949 ±0.04 8.943 ±0.09 1.528 ±0.02 1.526 ±0.01 1.463 ±0.04 5.823 ±0.00 1.339 ±0.01 0.140 ±0.00 0.167 ±0.00 9.434 ±0.05 3.167 ±1.71
#7515 23.178 ±0.05 7.946 ±0.02 8.920 ±0.03 1.540 ±0.03 1.499 ±0.00 1.427 ±0.01 5.772 ±0.08 1.309 ±0.02 0.138 ±0.00 0.164 ±0.00 9.849 ±0.04 1.972 ±0.01

lib/utils/did-you-mean.js Outdated Show resolved Hide resolved
lib/commands/run-script.js Outdated Show resolved Hide resolved
lib/npm.js Outdated Show resolved Hide resolved
lib/npm.js Outdated Show resolved Hide resolved
lib/utils/error-message.js Outdated Show resolved Hide resolved
lib/npm.js Outdated Show resolved Hide resolved
These will be used to generate error messages from both commands and the exit handler.

Also makes the did-you-mean function take a package so it can be sync and called more easily from the error handlers.
@lukekarrys lukekarrys changed the title fix(refactor): split error messaging into more fns fix(refactor): create new error output primitives May 13, 2024
@lukekarrys lukekarrys marked this pull request as ready for review May 13, 2024 06:19
@lukekarrys lukekarrys requested a review from a team as a code owner May 13, 2024 06:19
],
Array [
"404",
"Note that you can also install from a",
Copy link
Contributor Author

@lukekarrys lukekarrys May 13, 2024

Choose a reason for hiding this comment

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

These snapshot changes don't actually change what is displayed to the user. The snapshots changed because these are unit tests.

When we format these messages consecutive calls to output with the same first param (404 in this case) are rendered identically to one call with a message that includes newlines.

module.exports = {
outputError,
jsonError,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a separate file because these functions will get used in multiple places by #7508.

They are also separate from error-message.js because they need to be required eagerly to prevent the case of them being removed when npm updates itself.

@lukekarrys lukekarrys merged commit b54cdb8 into latest May 13, 2024
26 checks passed
@lukekarrys lukekarrys deleted the lk/errors-and-json branch May 13, 2024 17:24
@github-actions github-actions bot mentioned this pull request May 13, 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.

3 participants