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

Cache resolution of Node targets via fingerprinting lockfiles. #7414

Merged

Conversation

Projects
None yet
2 participants
@blorente
Copy link
Contributor

commented Mar 21, 2019

Problem

When working with a Node target, if the user made a change to a source file, it would re-trigger yarn install or similar installation.

Solution

Create a FingerprintStrategy for the NodeResolve task, which invalidates targets based solely on changes to a predefined list of lockfiles (currently, package.json and yarn.lock).

Result

Running:

$ ./pants run a/node/target
$ <edit a/node/target/index.js>
$ ./pants run a/node/target

doesn't trigger [resolve][node] anymore.

TODO:

  • Parametrerize the files to fingerprint into an option.

@blorente blorente requested review from stuhood and nsaechao Mar 21, 2019

@blorente blorente force-pushed the blorente:blorente/fingerprint-node-lockfiles branch from 0480dd0 to 50f59d2 Mar 21, 2019

@stuhood
Copy link
Member

left a comment

This looks great, thanks!

Ny is on vacation for a few weeks, but I think that you're good to land after the one comment.

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

So there's an unresolved semantics problem: What happens to npm targets? They don't have yarn.lock files. The solution is (since NodeResolver is already intentionally aware of the package_manager field) to determine which lockfiles to look for based on the package manager. However, that removes the option for us to do it via a cli option, since this is now a per-target thing.

So, the solution I'm going to try is:

  • Remove the option
  • For each target, default to reading the package manager and deciding which lockfiles to watch.
  • Add an optional field to NodePackage NodeModule targets to watch out for additional lockfiles.

@blorente blorente force-pushed the blorente:blorente/fingerprint-node-lockfiles branch from ca8b219 to 2c7b64a Mar 22, 2019

blorente added some commits Mar 22, 2019

@blorente blorente force-pushed the blorente:blorente/fingerprint-node-lockfiles branch from 9acda35 to 675caf8 Mar 25, 2019

@blorente blorente requested a review from stuhood Mar 25, 2019

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@stuhood I moved the cli option to a target field, I'd like another pair of eyes on it to see that there's nothing fundamentally broken about the approach, if that's okay.

@blorente blorente merged commit f85588e into pantsbuild:master Mar 26, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.