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): use full location as tracker key when inflating #4300

Merged
merged 1 commit into from
Feb 22, 2022

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jan 20, 2022

Fixes: #4273
Ref: #4298

I ran into this issue in #4427 while making tracker events run by default on all tests. This test was failing because the two packages being inflated are file: so they don't start with node_modules/ resulting in the location portion of the tracker key being empty. I don't see a reason why the key shouldn't be the full path, except that we show it to the user as part of the logs. But if that's a concern it can be fixed elsewhere.

@lukekarrys lukekarrys requested a review from a team as a code owner January 20, 2022 18:29
@npm-robot
Copy link
Contributor

npm-robot commented Jan 20, 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 73.688 ±3.58 39.321 ±0.57 23.760 ±0.24 27.453 ±1.40 3.924 ±0.21 3.734 ±0.03 3.022 ±0.00 16.036 ±0.21 3.035 ±0.02 4.364 ±0.08
#4300 0.527 ±0.01 0.521 ±0.00 0.520 ±0.00 0.539 ±0.02 0.512 ±0.00 0.533 ±0.02 0.535 ±0.04 0.512 ±0.00 0.529 ±0.03 0.540 ±0.04
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 47.599 ±0.86 29.887 ±0.04 18.121 ±0.31 18.682 ±0.18 3.534 ±0.02 3.500 ±0.01 3.172 ±0.05 12.524 ±0.00 2.930 ±0.04 3.940 ±0.05
#4300 0.522 ±0.01 0.528 ±0.01 0.544 ±0.01 0.539 ±0.04 0.544 ±0.00 0.564 ±0.04 0.523 ±0.00 0.524 ±0.00 0.515 ±0.01 0.518 ±0.00

@lukekarrys lukekarrys marked this pull request as draft February 3, 2022 17:56
@lukekarrys lukekarrys marked this pull request as ready for review February 22, 2022 06:49
@lukekarrys lukekarrys changed the title Fix Tracker "idealTree:inflate:" already exists fix(arborist): use full location as tracker key when inflating Feb 22, 2022
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.

It's Fine ™️ to show the full location here imo, but if we want to strip it I added a suggestion. Approved either way.

@lukekarrys
Copy link
Contributor Author

So I'm not exactly sure why but stripping out node_modules/ at all breaks that same test I referenced above. I logged the full location and the location with node_modules/ removed and found that in that test it is inflating a and b in two different locations that end up being the same. I'm not sure if this points to a deeper arborist bug, but I'm confident that this fix is a net positive.

location: a
wo_nodem: a
====================
location: a/node_modules/@ruyadorno/package-with-added-bin
wo_nodem: a/node_modules/@ruyadorno/package-with-added-bin
====================
location: b
wo_nodem: b
====================
location: b/node_modules/@ruyadorno/package-with-added-bin
wo_nodem: b/node_modules/@ruyadorno/package-with-added-bin
====================
location: node_modules/a
wo_nodem: a
====================
location: node_modules/b
wo_nodem: b
====================

FAIL  test/arborist/build-ideal-tree.js
✖ with lockfile

test: test/arborist/build-ideal-tree.js deduped transitive deps with asymmetrical bin declaration
at: {}
found:
  !error
  name: Error
  message: Tracker "idealTree:inflate:a" already exists
  stack: >-
    Error: Tracker "idealTree:inflate:a" already exists
        at Arborist.[_onError] (/Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/tracker.js:96:11)
        at Arborist.addTracker (/Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/tracker.js:25:21)
        at Array.<anonymous> (/Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/arborist/build-ideal-tree.js:761:14)
        at run (/Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/index.js:30:26)
        at /Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/index.js:39:5
        at new Promise (<anonymous>)
        at promiseCallLimit (/Users/lukekarrys/projects/npm/cli/node_modules/promise-call-limit/index.js:2:48)
        at Arborist.[inflateAncientLockfile] (/Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/arborist/build-ideal-tree.js:776:11)
        at Arborist.buildIdealTree (/Users/lukekarrys/projects/npm/cli/workspaces/arborist/lib/arborist/build-ideal-tree.js:214:13)

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.

5 participants