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: updated ebadplatform messaging to be generated based on the error #6277

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Mar 21, 2023

the changes here are meant to make it so that the npm cli doesn't have to know about what keys npm-install-checks sets in the current and required properties of the error. to facilitate this, the following changes to output have been made:

  • arch has been renamed to cpu to reflect the field in the package.json that is actually being checked
  • any fields that are undefined or a zero-length array in the error's required property are removed from the messaging. this can reduce the messaging so users are only shown things that the package actually asked for rather than letting undefined and empty strings through
  • field names are no longer uppercased in the detailed message, instead they are left as the error has them
  • Valid and Actual lines for the same key are now next to each other instead of having all of the Valids followed by all of the Actuals

Example of the previous valid/actual behavior

Valid OS:     darwin
Valid Arch:   arm64
Actual OS:    linux
Actual Arch:  x86

And the new behavior

Valid os:    darwin
Actual os:   linux
Valid cpu:   arm64
Actual cpu:  x86

@nlf nlf requested a review from a team as a code owner March 21, 2023 18:21
@nlf nlf requested review from fritzy and removed request for a team March 21, 2023 18:21
@npm-cli-bot
Copy link
Collaborator

found 1 benchmarks with statistically significant performance improvements

  • app-medium: no-clean

found 3 benchmarks with statistically significant performance regressions

  • app-large: lock-only, no-clean:audit, run-script
timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
show-version run-script
npm@9 43.636 ±0.45 23.387 ±0.65 22.303 ±0.68 25.458 ±0.16 3.758 ±0.07 4.180 ±0.05 3.158 ±0.10 16.406 ±0.31 3.300 ±0.03 4.684 ±0.03 0.645 ±0.00 0.608 ±0.01
#6277 48.352 ±1.32 26.854 ±0.68 24.464 ±0.12 27.497 ±0.45 4.107 ±0.05 4.277 ±0.05 3.342 ±0.05 16.862 ±0.08 3.391 ±0.00 5.282 ±0.52 0.667 ±0.01 0.686 ±0.02
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
show-version run-script
npm@9 32.910 ±1.19 18.920 ±0.18 17.809 ±0.11 18.495 ±0.61 3.648 ±0.01 3.756 ±0.06 3.206 ±0.06 12.043 ±0.17 3.226 ±0.00 4.394 ±0.02 0.661 ±0.02 0.661 ±0.02
#6277 31.900 ±0.41 19.005 ±0.07 17.430 ±0.28 18.554 ±0.59 3.807 ±0.01 3.865 ±0.07 3.178 ±0.01 11.398 ±0.33 2.806 ±0.15 4.324 ±0.01 0.673 ±0.01 0.685 ±0.01

@wraithgar wraithgar merged commit 2def359 into latest Mar 21, 2023
@wraithgar wraithgar deleted the nlf/ebadplatform-messaging branch March 21, 2023 19:08
@github-actions github-actions bot mentioned this pull request Mar 21, 2023
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