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(node): do NOT run "yarn/npm install" if package.json did not change #1435

Merged
merged 3 commits into from
Dec 30, 2021

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Dec 30, 2021

Run yarn/npm install in post-synthesis only if package.json had actually changed. Otherwise, there is no need.

If node_modules doesn't exist in the project root, we also install to allow users to simply run npx projen from freshly cloned repositories.

conditional-install.mov

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Run `yarn/npm install` in post-synthesis only if `package.json` had actually changed (or if `node_modules` doesn’t exist). Otherwise, there is no need.
@mergify mergify bot added the contribution/core ⚙️ used by automation label Dec 30, 2021
@eladb eladb changed the title feat(node): do not run “install” if package.json did not change feat(node): do run "yarn/npm install" if package.json did not change Dec 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2021

Codecov Report

Merging #1435 (d52fb0f) into main (d90284c) will decrease coverage by 0.29%.
The diff coverage is 93.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1435      +/-   ##
==========================================
- Coverage   88.06%   87.76%   -0.30%     
==========================================
  Files         132      134       +2     
  Lines        5109     5165      +56     
  Branches     1207     1198       -9     
==========================================
+ Hits         4499     4533      +34     
- Misses        610      632      +22     
Impacted Files Coverage Δ
src/awscdk/cdk-config.ts 100.00% <ø> (ø)
src/cdk/consts.ts 100.00% <ø> (+36.36%) ⬆️
src/github/github-project.ts 100.00% <ø> (ø)
src/javascript/node-package.ts 73.37% <71.42%> (-2.70%) ⬇️
src/javascript/node-project.ts 78.72% <83.33%> (-4.71%) ⬇️
src/javascript/prettier.ts 85.71% <85.71%> (ø)
src/awscdk/awscdk-app-ts.ts 92.53% <100.00%> (ø)
src/awscdk/cdk-tasks.ts 100.00% <100.00%> (ø)
src/build/build-workflow.ts 94.23% <100.00%> (-1.30%) ⬇️
src/cdk/jsii-project.ts 92.15% <100.00%> (-0.16%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a202663...d52fb0f. Read the comment docs.

Copy link
Contributor

@Chriscbr Chriscbr left a comment

Choose a reason for hiding this comment

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

Woah very cool!

One thought I had was whether there should be some kind of "--force" flag to override this behavior. But this might first require formalizing this post-installation behavior as an API.

I also wonder if it makes sense to also extend this behavior to the projenrc file? But for this to be reliable we would also probably have to check that tasks.json did not change and the projen dependency defined in package.json did not change.

@mergify mergify bot merged commit d193c1c into main Dec 30, 2021
@mergify mergify bot deleted the eladb/conditional-install branch December 30, 2021 16:28
@eladb
Copy link
Contributor Author

eladb commented Dec 30, 2021

One thought I had was whether there should be some kind of "--force" flag to override this behavior. But this might first require formalizing this post-installation behavior as an API.

I guess one can just run yarn --force no?

@eladb eladb changed the title feat(node): do run "yarn/npm install" if package.json did not change feat(node): do NOT run "yarn/npm install" if package.json did not change Jan 2, 2022
eladb pushed a commit that referenced this pull request Jan 2, 2022
#1435 introduced an optimization to only run `yarn/npm install` if package.json has changed. This uncovered a latent issue which already existed.

When a dependency version is not specified, projen initially synthesizes a `package.json` file with a `*` dependency. Then invokes `yarn install`, resolves the actual version from the installed module and updates `package.json` with a caret version requirement. This mimicks the same behavior as `yarn add` or `npm install`.

But now we need to re-execute "install" in order to update the package lock file.

Fixes #1443
mergify bot pushed a commit that referenced this pull request Jan 2, 2022
#1435 introduced an optimization to only run `yarn/npm install` if package.json has changed. This uncovered a latent issue which already existed.

When a dependency version is not specified, projen initially synthesizes a `package.json` file with a `*` dependency. Then invokes `yarn install`, resolves the actual version from the installed module and updates `package.json` with a caret version requirement. This mimicks the same behavior as `yarn add` or `npm install`.

But now we need to re-execute "install" in order to update the package lock file.

Fixes #1443

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core ⚙️ used by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants