fix(core): prefer project specific external deps #23307

JamesHenry · 2024-05-10T15:30:13Z

Current Behavior

We only ever discover one version of an external dependency for the file-map.json. This means the @nx/dependency-checks lint rule can produce incorrect failures when multiple copies of a dependency exist within a workspace.

Expected Behavior

In the file-map.json the project specific version of a package (if multiple exist) is preferred and therefore the lint rule produces accurate results.

For example, in a repo where the root package.json has lodash@4.0.0 (which becomes npm:lodash on graph) and the foo project has lodash@3.0.0:

Before

After

Performance

NX_ISOLATE_PLUGINS=true NX_PERF_LOGGING=true NX_DAEMON=false nx show project nx --json false

** Before **

Time for 'build typescript dependencies' 505.52144700009376

** After **

Time for 'build typescript dependencies' 701.247584999539

vercel · 2024-05-10T15:30:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
nx-dev	✅ Ready (Inspect)	Visit Preview	May 30, 2024 1:35pm

nx-cloud · 2024-05-10T15:30:54Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f960047. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

…s instead of semver matching

packages/nx/src/plugins/js/project-graph/build-dependencies/explicit-project-dependencies.ts

...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-package-json-dependencies.ts

packages/nx/src/plugins/js/project-graph/build-dependencies/build-dependencies.ts

JamesHenry · 2024-05-20T20:04:34Z

@FrozenPandaz Please take another look

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts

JamesHenry · 2024-05-21T17:08:55Z

packages/nx/src/plugins/js/utils/find-external-package-json-path.ts

+ * NOTE: Unit testing this code is currently impractical as it is not possible to mock
+ * require.resolve in jest https://github.com/jestjs/jest/issues/9543
+ */
+export function findExternalPackageJsonPath(


It exists in its own file so that it can be mocked in tests, per the note there we cannot mock the inner require.resolve calls so we need something we can mock that wraps it

Wait... rereading that:

I think this can move into the TargetProjectLocator

I understood this sentence to mean into the class implementation, but do you actually mean just into the target-project-locator.ts file? That is doable if so

Yeah, the file

This is not mockable when taking that approach because the function is referenced within the TargetProjectLocator directly so there is no longer an import boundary to mock. It needs to stay in a separate file.

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts

...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-project-dependencies.spec.ts

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts

...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-package-json-dependencies.ts

…ect-external-nodes

…d on import file path

….findProjectFromImport

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts

FrozenPandaz

We will want to profile how this does on the nx repo to ensure there isn't a large performance regression for finding dependencies.

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts

JamesHenry · 2024-05-30T13:26:39Z

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts

-      './libs/proj1234-child/index.ts': 'export const a = 12345',
-    };
-    vol.fromJSON(fsJson, '/root');
-    ctx = {


This was unused

JamesHenry · 2024-05-30T13:26:52Z

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts

-    expect(result2).toEqual('npm:npm-package');
-  });
-
-  it('should be able to resolve paths that have similar names', () => {


This was the test we agreed to remove

JamesHenry changed the title ~~Project external nodes~~ fix(core): prefer project specific external deps May 10, 2024

JamesHenry mentioned this pull request May 10, 2024

fix(core): prefer project specific external deps #22111

Closed

JamesHenry force-pushed the project-external-nodes branch 3 times, most recently from 4e86fce to fb21b87 Compare May 10, 2024 17:31

fix(core): prefer project specific external deps

f50d972

JamesHenry force-pushed the project-external-nodes branch from fb21b87 to f50d972 Compare May 10, 2024 18:14

JamesHenry marked this pull request as ready for review May 10, 2024 18:17

JamesHenry requested a review from a team as a code owner May 10, 2024 18:18

JamesHenry requested review from Cammisuli and FrozenPandaz and removed request for Cammisuli May 10, 2024 18:18

JamesHenry self-assigned this May 10, 2024

JamesHenry enabled auto-merge (squash) May 11, 2024 09:38

JamesHenry added 3 commits May 14, 2024 23:15

Merge branch 'master' into project-external-nodes

d40ec61

chore(core): refactor to use resolved external node package.json file…

1c39578

…s instead of semver matching

Merge branch 'master' into project-external-nodes

f2ecf1a

vercel bot deployed to Preview May 15, 2024 14:18 View deployment

FrozenPandaz requested changes May 15, 2024

View reviewed changes

JamesHenry added 2 commits May 16, 2024 17:01

fix(core): always use disk as source of truth

ad7bff0

chore(repo): tmp disable failing tests to try CI

a5faada

JamesHenry requested a review from a team as a code owner May 16, 2024 13:02

JamesHenry requested a review from meeroslav May 16, 2024 13:02

JamesHenry marked this pull request as draft May 16, 2024 13:03

auto-merge was automatically disabled May 16, 2024 13:03
Pull request was converted to draft

JamesHenry added 2 commits May 17, 2024 13:02

chore(repo): updates

d59a91d

Merge branch 'master' into project-external-nodes

644bec4

vercel bot deployed to Preview May 17, 2024 09:12 View deployment

Merge branch 'master' into project-external-nodes

21aa145

vercel bot deployed to Preview May 17, 2024 13:13 View deployment

JamesHenry marked this pull request as ready for review May 17, 2024 14:17

JamesHenry requested a review from a team as a code owner May 17, 2024 14:17

JamesHenry requested a review from jaysoo May 17, 2024 14:17

Merge branch 'master' into project-external-nodes

1d05097

vercel bot deployed to Preview May 17, 2024 14:49 View deployment

Merge branch 'master' into project-external-nodes

d632383

vercel bot deployed to Preview May 17, 2024 16:43 View deployment

Merge branch 'master' into project-external-nodes

51cbac6

vercel bot deployed to Preview May 17, 2024 18:49 View deployment

JamesHenry requested a review from FrozenPandaz May 20, 2024 20:04

FrozenPandaz requested changes May 21, 2024

View reviewed changes

JamesHenry added 5 commits May 23, 2024 15:44

chore(core): pr feedback

f8c457c

chore(core): fix formatting

1785b3d

Merge branch 'project-external-nodes' of github.com:nrwl/nx into proj…

2422a34

…ect-external-nodes

chore(core): TargetProjectLocator.findNpmProjectFromImport works base…

9839d01

…d on import file path

chore(core): remove the need for project root in TargetProjectLocator…

565d1a8

….findProjectFromImport

JamesHenry commented May 23, 2024

View reviewed changes

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts Outdated Show resolved Hide resolved

JamesHenry requested a review from FrozenPandaz May 23, 2024 13:37

FrozenPandaz requested changes May 27, 2024

View reviewed changes

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts Outdated Show resolved Hide resolved

packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts Outdated Show resolved Hide resolved

JamesHenry added 2 commits May 30, 2024 17:24

chore(core): pr feedback

4b8338e

Merge branch 'master' into project-external-nodes

f960047

JamesHenry commented May 30, 2024

View reviewed changes

vercel bot deployed to Preview May 30, 2024 13:35 View deployment

FrozenPandaz approved these changes May 30, 2024

View reviewed changes

FrozenPandaz merged commit 7001e35 into master May 30, 2024
6 checks passed

FrozenPandaz deleted the project-external-nodes branch May 30, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): prefer project specific external deps #23307

fix(core): prefer project specific external deps #23307

JamesHenry commented May 10, 2024 •

edited by FrozenPandaz

vercel bot commented May 10, 2024 •

edited

nx-cloud bot commented May 10, 2024 •

edited

JamesHenry commented May 20, 2024

This comment was marked as resolved.

JamesHenry May 21, 2024 •

edited

JamesHenry May 21, 2024

FrozenPandaz May 22, 2024

JamesHenry May 23, 2024 •

edited

FrozenPandaz left a comment

JamesHenry May 30, 2024

JamesHenry May 30, 2024

fix(core): prefer project specific external deps #23307

fix(core): prefer project specific external deps #23307

Conversation

JamesHenry commented May 10, 2024 • edited by FrozenPandaz

Current Behavior

Expected Behavior

Performance

vercel bot commented May 10, 2024 • edited

nx-cloud bot commented May 10, 2024 • edited

☁️ Nx Cloud Report

JamesHenry commented May 20, 2024

This comment was marked as resolved.

JamesHenry May 21, 2024 • edited

Choose a reason for hiding this comment

JamesHenry May 21, 2024

Choose a reason for hiding this comment

FrozenPandaz May 22, 2024

Choose a reason for hiding this comment

JamesHenry May 23, 2024 • edited

Choose a reason for hiding this comment

FrozenPandaz left a comment

Choose a reason for hiding this comment

JamesHenry May 30, 2024

Choose a reason for hiding this comment

JamesHenry May 30, 2024

Choose a reason for hiding this comment

JamesHenry commented May 10, 2024 •

edited by FrozenPandaz

vercel bot commented May 10, 2024 •

edited

nx-cloud bot commented May 10, 2024 •

edited

JamesHenry May 21, 2024 •

edited

JamesHenry May 23, 2024 •

edited