Plugin: Pin dependencies that slipped into trunk

[tomasztunik]

Update

Leaving old reasoning for reference.

The error that we've seen that we have attempted to fix with change to npm ci from npm install in the Static Check workflow was actually caused by unpinned dependency getting out of sync and causing errors with new package-lock getting generated and exiting with an error because of that. Which is actually correct behaviour as packages should always have their versions pinned.

I've pinned the offending packages to the versions that are currently in the package-lock and updated the comment on the workflow to make it more explicit that this is also the responsibility of this step.

What?

Revert Static Analysis workflow to use npm ci again.

Fixes #16157
Blocks #39560

Why?

Because we've used npm intall - which regenerates package-lock.json - we've had a rare case of Static Analysis verification step fail on dependency update because before dependency PR was merged new version was released and newly created package-lock in the step did not match the one that was contributed by the dependabot. (Ref issue log).

When issue that made us use npm install over ci was present travis used latest LTS available - which was Node v10. Now we are using Node v14 and in the meantime the issue with npm ci not reporting errors correctly got fixed.

The problem mentioned in #16157 is not reproductible anymore and using npm install instead of npm ci can cause another set of issues as documented above.

How?

We are updating .github/workflows/static-checks.yml to use npm ci.

Testing Instructions

As per #16157:

git checkout 5cf4e14~1 # The commit prior to 14808
npm ci
echo $?

This should produce non zero output - verified to produce 1

[noahtallen]

The only thing that comes to mind is whether or not this check:

There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!

would now detect any "legitimate" issues.

[gziolo]

Y

The only thing that comes to mind is whether or not this check:

There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!

would now detect any "legitimate" issues.

I don’t know about now, but in the past npm ci wouldn’t report any changes to the lock file that npm install applies. That’s way we went explicitly for npm install. The issue was that too many contributors would include the duplicated accidental lock file changes in their PRs.

[tomasztunik]

There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!

check-local-changes check should now throw only if there are unexpected changes produced by docs:build that have not been committed.

I believe that before with npm install in this step it would mean that it could potentially force user to do minor version updates on packages if it detected that npm install in this check would have produced changes - like in the case of the linked failed package update PR it now says - hey there is newer stuff than what you are pushing. Potentially forcing user to have to do a package update that is not even related to the surface of their change. Am I right here?

Is the intention of this step is to ensure Gutenberg is always using the latest version of package-lock according to set versioning rules for packages and everybody is responsible for updating dependencies even if they are not related to their change?

If this is the case than npm install should stay else I think we should go with npm ci and maybe update the mentioned check-local-changes to not warn about possible changes introduced by npm install (overlooked this one)

Personally I prefer "concious" package updates and it's easy to ignore minor updates in unrelated areas of the code potentially messing things up. It's too easy to just go oh I'll just run npm install and be done with it, without actually checking what got updated and why. And there are still packages that do not follow semver ie. WordPress :)

npm ci would throw only if:

• package is missing in the repository/not accessible
• locked version is not available in the repo anymore
• there was some kind of error causing non 0 exit when installing packages from the lockfile

So this would protect us only from dependencies for any reason not being available anymore not about new updates to lockfile being available.

[gziolo]

@tomasztunik, I don't think that what you described is the motivation for the CI check for the lock file. The project doesn't use ranges for npm dependencies so you shouldn't get random updates by running npm install. In practice, ranges are used for WordPress packages to make them usable, but still it doesn't trigger random version updates in the lock file.

It's all about ensuring that the lock file is predictable and it looks the same for everyone. CI becomes the source of truth because for every PR it runs npm install and its result is used as a reference. This way for a given set of changes to the dependencies it will ensure that the lock file produced won't change when somone runs npm install locally.

[tomasztunik]

Ah this makes sense now. So the issue seems to be some packages have slipped the rule of having pinned versions.

		"@emotion/native": "^11.0.0",
		"@testing-library/user-event": "^14.0.0-beta.13",
		"filenamify": "^4.2.0",
		"simple-git": "^2.35.0",

and the dependency from linked broken PR is on this list.

Should I roll back npm ci change and pin the packages?

[gziolo]

Good catch! I see now that some packages use ranges. Yes, I think it's a good idea and we should pin those dependencies. Maybe extending the CI check to prevent using ranges is a reasonable follow-up as that would prevent the issue in the first place.

[tomasztunik]

What do you think we update .npmrc to include save-exact=true? I believe most of these slips come from save-exact being false by default.

Also there is check-exact package that verifies if versions are pinned, and that's including npm dependencies, github links etc. Plays nice with husky as well.

[gziolo]

What do you think we update .npmrc to include save-exact=true? I believe most of these slips come from save-exact being false by default.

Also there is check-exact package that verifies if versions are pinned, and that's including npm dependencies, github links etc. Plays nice with husky as well.

As long as it applies to the project, but not the individual WordPress packages then it would be a great addition 😄

[tomasztunik]

Will have to look into it bit more after hours and prepare the follow up if I found a good way to handle it, noticed that the setting is already in .npmrc in the root but seems it did not work somehow. Unless someone added the deps by hand 🤷.

Pushed revert of npm ci and updated the comment. Updated the PR description to reflect the discussion. Pinned the packages, and pushed new lock. Updated the failing #39560 simple-git package to the version I've verified works ok as well so we can close the blocked dependabot PR with this one providing more recent version.

Should #16157 be closed as having npm install is not an issue as discussed here? We want npm install to stay as a way to ensure dependencies are correct and following the link in the comment might give wrong impression.

[gziolo]

https://github.com/steveukx/git-js/releases/tag/simple-git-v3.0.0

It doesn't look like there are breaking changea that could impact the project.

[tomasztunik]

As mentioned here I actually tested it against our repo and it works good 👌.

[gziolo]

We use simple-git for example for publishing WordPress packages to npm so you couldn't test it. I wanted to ensure they didn't change any APIs. We will see this Friday 🤣

[tomasztunik]

From what I've seen to some extent wp-env was also using it for setting things up - so was testing it on this end :)

[gziolo]

It looks good. Thank you for going through the whole process of finding the final solution.

[gziolo]

Should #16157 be closed as having npm install is not an issue as discussed here? We want npm install to stay as a way to ensure dependencies are correct and following the link in the comment might give wrong impression.

I belive we should for now. We can reevaluate everything once we finally switch to npm 8. npm 6 had so many bugs that forced us to use this suboptimal solution in the first place.

Will have to look into it bit more after hours and prepare the follow up if I found a good way to handle it, noticed that the setting is already in .npmrc in the root but seems it did not work somehow. Unless someone added the deps by hand 🤷.

It's nice to have. Maybe we can have something very simple that iterates over all dependencies and checks wheter it's a pinned version.