-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(core): prefer project specific external deps #23307
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
☁️ Nx Cloud ReportCI 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. |
4e86fce
to
fb21b87
Compare
fb21b87
to
f50d972
Compare
packages/nx/src/plugins/js/project-graph/build-dependencies/explicit-project-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/explicit-project-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/explicit-project-dependencies.ts
Outdated
Show resolved
Hide resolved
...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-package-json-dependencies.ts
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/build-dependencies.ts
Outdated
Show resolved
Hide resolved
@FrozenPandaz Please take another look |
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts
Outdated
Show resolved
Hide resolved
* 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( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-project-dependencies.spec.ts
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.ts
Show resolved
Hide resolved
...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-package-json-dependencies.ts
Outdated
Show resolved
Hide resolved
...ges/nx/src/plugins/js/project-graph/build-dependencies/explicit-package-json-dependencies.ts
Show resolved
Hide resolved
…ect-external-nodes
…d on import file path
….findProjectFromImport
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
packages/nx/src/plugins/js/project-graph/build-dependencies/target-project-locator.spec.ts
Outdated
Show resolved
Hide resolved
'./libs/proj1234-child/index.ts': 'export const a = 12345', | ||
}; | ||
vol.fromJSON(fsJson, '/root'); | ||
ctx = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was unused
expect(result2).toEqual('npm:npm-package'); | ||
}); | ||
|
||
it('should be able to resolve paths that have similar names', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the test we agreed to remove
<!-- Please make sure you have read the submission guidelines before posting an PR --> <!-- https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr --> <!-- Please make sure that your commit message follows our format --> <!-- Example: `fix(nx): must begin with lowercase` --> ## Current Behavior <!-- This is the behavior we have today --> 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 <!-- This is the behavior we should expect with the changes in this PR --> 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** ![image](https://github.com/nrwl/nx/assets/900523/fa0b3711-5004-4abc-9904-0433a37bc3cf) **After** ![image](https://github.com/nrwl/nx/assets/900523/d0527368-44b4-486f-aa7a-79d996a20ef4) ## 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 (cherry picked from commit 7001e35)
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
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