Skip to content

fix(arborist): prioritize valid workspace nodes#4230

Merged
lukekarrys merged 1 commit intorelease-nextfrom
nlf/arborist-workspace-priority
Jan 20, 2022
Merged

fix(arborist): prioritize valid workspace nodes#4230
lukekarrys merged 1 commit intorelease-nextfrom
nlf/arborist-workspace-priority

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 •
edited
Loading

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
fritzy
fritzy reviewed Jan 13, 2022
View reviewed changes
workspaces/arborist/test/arborist/build-ideal-tree.js Outdated
],
})

t.matchSnapshot(printTree(tree))
Copy link

@fritzy fritzy Jan 13, 2022

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

@ljharb ljharb Jan 13, 2022

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
Merged
@npm-robot npm-robot mentioned this pull request Jan 20, 2022
Closed
@hzuo hzuo mentioned this pull request Jan 22, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@ljharb ljharb ljharb left review comments

@fritzy fritzy fritzy left review comments

@lukekarrys lukekarrys lukekarrys approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nlf @npm-robot @ljharb @fritzy @lukekarrys