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(arborist): prioritize valid workspace nodes #4230

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Jan 11, 2022

closes #3637

@nlf nlf requested a review from a team as a code owner January 11, 2022 23:58
@npm-robot
Copy link
Contributor

npm-robot commented Jan 12, 2022

found 20 benchmarks with statistically significant performance improvements

  • app-large: clean, lock-only, cache-only, cache-only:peer-deps, modules-only, no-lock, no-cache, no-modules, no-clean, no-clean:audit
  • app-medium: clean, lock-only, cache-only, cache-only:peer-deps, modules-only, no-lock, no-cache, no-modules, no-clean, no-clean:audit
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
npm@8 55.702 ±0.61 33.505 ±0.48 36.816 ±23.21 22.226 ±1.13 3.435 ±0.01 3.289 ±0.08 2.649 ±0.04 12.843 ±0.23 2.634 ±0.10 3.730 ±0.16
#4230 0.450 ±0.01 0.448 ±0.01 0.445 ±0.01 0.450 ±0.00 0.467 ±0.02 0.448 ±0.01 0.470 ±0.01 0.448 ±0.00 0.449 ±0.00 0.448 ±0.00
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 40.896 ±0.95 25.372 ±0.06 14.180 ±0.02 15.631 ±0.03 3.048 ±0.04 3.091 ±0.12 2.727 ±0.02 9.799 ±0.16 2.436 ±0.02 3.332 ±0.10
#4230 0.457 ±0.02 0.476 ±0.03 0.460 ±0.01 0.475 ±0.01 0.457 ±0.01 0.446 ±0.00 0.460 ±0.00 0.464 ±0.02 0.460 ±0.02 0.459 ±0.03

@nlf nlf added Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes ws:arborist Related to the arborist workspace release: next These items should be addressed in the next release labels Jan 12, 2022
],
})

t.matchSnapshot(printTree(tree))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish this test had assertions to make it more explicit on the behavior you're looking for.

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 specifically "the output is tab-indented" - other than ensuring that each line starts with either "not a space" or "a tab", i'm not sure what i'd do. what would you prefer?

@nlf nlf force-pushed the nlf/arborist-workspace-priority branch from 8e0b41d to c76642a Compare January 13, 2022 21:08
@lukekarrys lukekarrys self-requested a review January 20, 2022 20:36
@lukekarrys lukekarrys merged commit c99c215 into release-next Jan 20, 2022
@lukekarrys lukekarrys deleted the nlf/arborist-workspace-priority branch January 20, 2022 20:37
@lukekarrys lukekarrys mentioned this pull request Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes ws:arborist Related to the arborist workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants