Skip to content

Ensure node_modules is up to date before gulp [default | css | build | dist] - #12586

Merged
rsimha merged 10 commits into
ampproject:masterfrom
rsimha:2017-12-21-YarnCheck
Jan 2, 2018
Merged

Ensure node_modules is up to date before gulp [default | css | build | dist]#12586
rsimha merged 10 commits into
ampproject:masterfrom
rsimha:2017-12-21-YarnCheck

Conversation

@rsimha

@rsimha rsimha commented Dec 21, 2017

Copy link
Copy Markdown
Contributor

This PR adds a new gulp update-packages task that's run as a prerequisite to:

gulp
gulp css
gulp build
gulp dist

It does the following:

  1. Runs yarn check --integrity to check if node_modules is in sync with the contents of package.json and yarn.lock. Typically, this will fail when git pull / git fetch causes an update to the set of dependencies.
  2. If step 1 fails, runs yarn check -- verify-tree to print a list of packages that need to be upgraded.
  3. If step 1 fails, runs yarn to bring all packages in node_modules up to date with package.json.

In the normal case, where node_modules is up to date (step 1 passes), this task adds less than 1 second to the gulp execution time. In the exceptional case, it takes a couple of seconds to enumerate all the packages that are out of date, and a fraction of a minute to run yarn, after which it will execute the gulp [default | css | build | dist] task that was initially invoked.

Partial fix for #12181

@rsimha

rsimha commented Dec 21, 2017

Copy link
Copy Markdown
Contributor Author

@erwinmombay @choumx @cramforce with this, we can ensure that all developers are running with the latest dependency versions.

Do you think the check is reasonable? Or do you think it's too stringent?

@cramforce

Copy link
Copy Markdown
Member

Could we just run yarn in the failure case?

@rsimha

rsimha commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

@cramforce that would be one step more aggressive than what I'm doing in the PR so far. Can do if you're okay with the contents of node_modules potentially being replaced.

@cramforce

Copy link
Copy Markdown
Member

I see no downside. You can also merge this first, of course.

@rsimha

rsimha commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

@cramforce PR updated with a call to yarn if packages are outdated. PTAL.

Comment thread build-system/tasks/yarn-check.js Outdated
/**
* Does a yarn check on node_modules, and if it is outdated, runs yarn.
*/
function yarnCheck() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the task now be updated to reflect it no longer just checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rsimha

rsimha commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

Tested locally on linux with:

  1. Local commits that change version numbers in package.json.
  2. Scenarios involving syncing forward and backward w.r.t. origin/master and / or upstream/master
  3. Branch switching, where different local git branches use different versions

Looking good. gulp update-packages typically takes less than half a second when packages are up to date, and less than 10 seconds when packages need an update.

@rsimha

rsimha commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

@cramforce Tested on mac os as well. Times are slightly slower than on linux, but nothing out of the ordinary. Once you approve, this is ready to be merged.

@rsimha

rsimha commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

And on Travis, since node_modules is always up to date, gulp update-packages adds less than 1 second to build time.

@rsimha

rsimha commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

Tested gulp update-packages by running gulp with different versions of yarn.

  • Recent versions of yarn (1.2.0 and above) will print a list of packages that need an update, update them by running yarn, and then proceed with running the gulp task.
  • Older, unsupported versions (like 1.1.0) will print a list of packages that need an update, then print a warning during yarn install with instructions for upgrading yarn, then attempt to update packages by running yarn, and finally run the gulp task.
  • Really old versions (like 0.17.8) will not be able print the list of packages that need an update, but they too will print a warning, attempt to update packages, and run the gulp task

@rsimha

rsimha commented Jan 2, 2018

Copy link
Copy Markdown
Contributor Author

Bumping this to the top. All comments addressed.

@rsimha
rsimha merged commit 14cf9a5 into ampproject:master Jan 2, 2018
@rsimha
rsimha deleted the 2017-12-21-YarnCheck branch January 2, 2018 17:40
@cramforce

Copy link
Copy Markdown
Member

We could skip this on CI.

@rsimha

rsimha commented Jan 2, 2018

Copy link
Copy Markdown
Contributor Author

gulp update-packages is a no-op during CI, since yarn check --integrity merely does a hash check that's always expected to pass (takes < 0.5 s to execute).

We have had instances in the past where people checked in mismatched versions of yarn.lock and package.json. Now that we safeguard against this, I believe we're okay leaving things as they are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants