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

feat(core): map lock file data to external dependencies #12185

Merged
merged 23 commits into from
Oct 6, 2022

Conversation

meeroslav
Copy link
Contributor

@meeroslav meeroslav commented Sep 22, 2022

This PR brings accurate external dependencies mapping to the project graph.

Implementation details

  • Parses lock file to LockFileData structure when a lock file is changed
  • Stores lock file hash in the nxdeps.json cache for later comparison (add an additional field to nx deps cache object)
  • Maps LockFileData to partial ProjectGraph (graph with only externalNodes and their dependencies populated)
  • Modifies project graph building to re-use the partial ProjectGraph while building the graph
  • Changes print-affected to ignore externalNodes's dependencies
  • Modifies the hasher to exclude lockfile file-based hashing and includes hashing of the task target
  • Extends hasher's external nodes hashing to traverse nested dependencies
  • Fixes several rigid tests

Benefits

  • Project Graph contains accurate information about packages and versions used
  • Hasher uses an accurate dependency tree
  • The package json generator uses a fixed version instead of ranges
  • Opens the possibility to fix several issues (e.g. Imports of lazy loaded libraries forbidden not triggered #11178)
  • Fixes issues that were blocked by missing dependencies

Performance (calculated on Nx repo) with Yarn:

  • Compare with cached lock file: 0.039ms
  • Full lock file parsing: ~150ms (due to cache miss)
    • Parse Syml (original function from yarn): ~124ms (point for replacement)

      This is should run in the background daemon so practically it shouldn't be noticeable, but nevertheless should still be improved

    • Hash Lock: 0.7ms
    • Map Packages: 25ms (point for improvement)
    • Detect root packages after mapping: 2-3ms (Yarn and Pnpm only, Npm has clean differentiation built in)

Fixes #12033
Fixes #11874

@meeroslav meeroslav self-assigned this Sep 22, 2022
@meeroslav meeroslav added the scope: core core nx functionality label Sep 22, 2022
@vercel
Copy link

vercel bot commented Sep 22, 2022

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

Name Status Preview Updated
nx-dev ✅ Ready (Inspect) Visit Preview Oct 5, 2022 at 9:45PM (UTC)

@meeroslav meeroslav marked this pull request as ready for review September 27, 2022 15:23
@meeroslav meeroslav changed the title feat(core): cache lock file data feat(core): cache lock file data WIP Sep 28, 2022
@meeroslav meeroslav marked this pull request as draft September 28, 2022 19:51
@meeroslav meeroslav changed the title feat(core): cache lock file data WIP feat(core): map lock file data to external dependencies Sep 30, 2022
@meeroslav meeroslav marked this pull request as ready for review September 30, 2022 09:48
});
}

private hashTarget(projectName: string, targetName: string): PartialHash {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has to be the other way around.

We don't know what your executor depends on. It's not like run-commands is an exception. It's the opposite: nrwl/webpack etc are exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It searches for executors in the externalNodes and if found returns the hash of that package and its dependencies. For run-commands we know we shouldn't look for the dependencies of @nrwl/workspace.

Are you saying that everything that isn't @nrwl/* should be considered as hash-all-external-nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@@ -361,6 +361,48 @@ class TaskHasher {
};
}

private traverseExternalNodesDependencies(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the number of external dependencies can be large and we traverse them for every task, should we cache the hash for every external dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be a good idea. To hash it during the parse phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added hash caching in the externalNodes

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
scope: core core nx functionality type: feature
Projects
None yet
2 participants