Skip to content

fix(arborist): use full location as tracker key when inflating#4300

Merged
lukekarrys merged 1 commit intorelease-nextfrom
fix-idealtree-inflate
Feb 22, 2022
Merged

fix(arborist): use full location as tracker key when inflating#4300
lukekarrys merged 1 commit intorelease-nextfrom
fix-idealtree-inflate

Conversation

@lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jan 20, 2022 •
edited
Loading

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 •
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 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 force-pushed the fix-idealtree-inflate branch from d1a710d to 830b987 Compare February 22, 2022 06:49
@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
wraithgar
wraithgar reviewed Feb 22, 2022
View reviewed changes
wraithgar
wraithgar approved these changes Feb 22, 2022
View reviewed changes
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

lukekarrys commented Feb 22, 2022

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)

@lukekarrys lukekarrys merged commit 9bdd1ac into release-next Feb 22, 2022
@lukekarrys lukekarrys deleted the fix-idealtree-inflate branch February 22, 2022 16:49
@lukekarrys lukekarrys mentioned this pull request Feb 24, 2022
Merged
@npm-robot npm-robot mentioned this pull request Feb 24, 2022
Closed
@wraithgar wraithgar mentioned this pull request Mar 15, 2022
Closed
1 task
@geriux geriux mentioned this pull request Nov 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wraithgar wraithgar wraithgar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lukekarrys @npm-robot @wraithgar @ruyadorno @kirtangajjar