Framework: Add package-lock precommit check for latest NPM #21306

aduth · 2020-03-31T23:17:14Z

This pull request seeks to add a pre-commit task which verifies that any changes to the root package-lock.json can only be committed if the local NPM version matches the latest available NPM version. This is in line with the documented required environment, and is intended to resolve cases where package-lock.json is wrongly updated with invalid values due to use of outdated version of NPM (see #16157 (comment)).

Testing Instructions:

Check out the branch
- git checkout add/latest-npm-package-lock-check
Install a version of NPM older than the current version
- npm i -g npm@6.14.3
Modify package-lock.json
- echo "example" >> package-lock.json
Add and attempt to commit the change
- git add . && git commit -m "test"
Verify that a failure occurs in the precommit and an instructional message is shown
Install the latest version of NPM
- npm i -g npm@latest
Attempt again to commit the change
- git commit -m "test"
Verify that the commit succeeds

github-actions · 2020-03-31T23:23:19Z

Size Change: +279 B (0%)

Total Size: 890 kB

Filename	Size	Change
`build/block-editor/index.js`	103 kB	+386 B (0%)
`build/block-library/index.js`	110 kB	+20 B (0%)
`build/block-library/style-rtl.css`	7.42 kB	-116 B (1%)
`build/block-library/style.css`	7.43 kB	-119 B (1%)
`build/dom/index.js`	3.1 kB	+48 B (1%)
`build/editor/editor-styles-rtl.css`	428 B	+29 B (6%)	🔍
`build/editor/editor-styles.css`	431 B	+31 B (7%)	🔍

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.4 kB	0 B
`build/api-fetch/index.js`	3.79 kB	0 B
`build/autop/index.js`	2.58 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.03 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-editor/style-rtl.css`	10.2 kB	0 B
`build/block-editor/style.css`	10.2 kB	0 B
`build/block-library/editor-rtl.css`	7.22 kB	0 B
`build/block-library/editor.css`	7.22 kB	0 B
`build/block-library/theme-rtl.css`	669 B	0 B
`build/block-library/theme.css`	671 B	0 B
`build/block-serialization-default-parser/index.js`	1.65 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	57.5 kB	0 B
`build/components/index.js`	195 kB	0 B
`build/components/style-rtl.css`	16.6 kB	0 B
`build/components/style.css`	16.5 kB	0 B
`build/compose/index.js`	6.21 kB	0 B
`build/core-data/index.js`	10.7 kB	0 B
`build/data-controls/index.js`	1.04 kB	0 B
`build/data/index.js`	8.23 kB	0 B
`build/date/index.js`	5.36 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/edit-navigation/index.js`	2.75 kB	0 B
`build/edit-navigation/style-rtl.css`	279 B	0 B
`build/edit-navigation/style.css`	280 B	0 B
`build/edit-post/index.js`	92.9 kB	0 B
`build/edit-post/style-rtl.css`	12.3 kB	0 B
`build/edit-post/style.css`	12.3 kB	0 B
`build/edit-site/index.js`	10.1 kB	0 B
`build/edit-site/style-rtl.css`	5.02 kB	0 B
`build/edit-site/style.css`	5.02 kB	0 B
`build/edit-widgets/index.js`	7.17 kB	0 B
`build/edit-widgets/style-rtl.css`	3.74 kB	0 B
`build/edit-widgets/style.css`	3.73 kB	0 B
`build/editor/index.js`	42.8 kB	0 B
`build/editor/style-rtl.css`	3.49 kB	0 B
`build/editor/style.css`	3.49 kB	0 B
`build/element/index.js`	4.44 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	6.95 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	1.93 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.3 kB	0 B
`build/keycodes/index.js`	1.7 kB	0 B
`build/list-reusable-blocks/index.js`	2.99 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	4.84 kB	0 B
`build/notices/index.js`	1.57 kB	0 B
`build/nux/index.js`	3.01 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.54 kB	0 B
`build/primitives/index.js`	1.5 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.5 kB	0 B
`build/server-side-render/index.js`	2.54 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.01 kB	0 B
`build/viewport/index.js`	1.6 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

bin/check-latest-npm.js

package.json

gziolo · 2020-04-01T14:25:24Z

What will happen when you are offline? Will it prevent committing changes? It might be an acceptable trade-off but I wanted to make sure we are fully aware of it. Do you happen to know if all other precommit checks work offline?

aduth · 2020-04-01T15:10:58Z

What will happen when you are offline? Will it prevent committing changes? It might be an acceptable trade-off but I wanted to make sure we are fully aware of it. Do you happen to know if all other precommit checks work offline?

That's a very good point which I hadn't considered.

I don't think we'd want to forbid someone from being able to work effectively while offline. At the same time, if we consider this to be important to avoiding the introduction of problematic changes, then we want to have high confidence they're running the correct version. The current proposed implementation otherwise blocks a commit in other cases of an inability to retrieve details about the package, so there's some precedent from that we could use.

Still, I might want to explore...

To see if there is any "cached" data available locally (e.g. if npm view npm would be cached) to use while offline
To warn but not block the commit, so at least the committer is informed of the potential problem

aduth · 2020-04-01T15:11:56Z

The build failures here are quite interesting. There appears to be some incompatibility with our current TypeScript configuration and the new dependency update I included here.

(cc @sirreal , more of a heads up. I haven't had a chance yet to dive into it)

sirreal · 2020-04-01T15:28:22Z

Error is here:

node_modules/chalk/index.d.ts(403,16): error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

Looks like they're using some constructs in the declaration that prohibit isolatedModules.

chalk/chalk#382

It's fine to disable that compiler option for bin typechecking because we won't be compiling anything there:

gutenberg/bin/tsconfig.json

Lines 3 to 15 in 4f144b4

    
           "compilerOptions": { 
        
           	"module": "commonjs", 
        
           	"esModuleInterop": true, 
        
           	"target": "ES6", 
        
           	"lib": [ "ES6", "ES2020.string" ], 
        
           	"rootDir": ".", 
        
           	"declarationMap": false, 
        
           	// We're not interested in the output, but we must generate 
        
           	// something as part of a composite project. Use the 
        
           	// ignored `.cache` file to hide tsbuildinfo and d.ts files. 
        
           	"outDir": ".cache" 
        
           },

aduth · 2020-04-02T13:06:51Z

I updated to override isolatedModules in 25b2cb5.

swissspidy · 2020-04-03T13:12:14Z

How does this work when not being connected to the internet?

aduth · 2020-04-03T13:13:04Z

@swissspidy See #21306 (comment)

swissspidy · 2020-04-03T13:17:37Z

D'oh, thanks! I swear I used CMD+F to search for this...

aduth · 2020-04-03T13:27:02Z

Reflecting some more, I'm thinking: We should probably still block the commit by default. In these specific circumstances, we can include additional messaging about why it's enforced, and how to work around it (i.e. git commit --no-verify).

aduth · 2020-04-07T21:46:42Z

Reflecting some more, I'm thinking: We should probably still block the commit by default. In these specific circumstances, we can include additional messaging about why it's enforced, and how to work around it (i.e. git commit --no-verify).

Updated in aac7243

Here's how it reads:

Latest NPM check failed!

Error: Could not contact the NPM registry to determine latest version.

This could be due to an intermittent outage of the service, or because you are not connected to the internet.

Because it is important that `package-lock.json` files only be committed while running the latest version of NPM, this commit has been blocked.

If you are certain of your changes and desire to commit anyways, you should either connect to the internet or bypass commit verification using git commit --no-verify.

This should be ready for a final review now.

aduth · 2020-04-08T13:43:27Z

To see if there is any "cached" data available locally (e.g. if npm view npm would be cached) to use while offline

This is actually the case:

⇒ npm view npm version
npm WARN registry Using stale data from https://registry.npmjs.org/ because the host is inaccessible -- are you offline?
npm WARN registry Using stale data from https://registry.npmjs.org/ due to a request error during revalidation.
6.14.4

We can strip the warnings to get the raw value:

⇒ npm view npm version --silent
6.14.4

I'm a bit reluctant still, partly out of effort required to revise the implementation, but also because: Per the warning message, this data can be stale, which contradicts what I'd proposed in #21306 (comment) and aac7243, where we enforce live data as a guarantee to ensure the latest version is known.

I'm inclined to keep things as they are, but we could always reconsider this in the future if it becomes a problem. Happy to hear other thoughts as well.

package-lock.json

gziolo · 2020-04-08T14:00:04Z

I'm inclined to keep things as they are, but we could always reconsider this in the future if it becomes a problem. Happy to hear other thoughts as well.

The issue is that when someone is offline, they still can use --no-verify to commit and we are back to square one :) I'm fine with whatever you decide as it will bring the number of issues to the number close to zero 👍

gziolo

Thank you for working on it 👍

To remove redundant chalk dependency entries (resolved instead from root)

aduth · 2020-04-08T18:24:07Z

Error is here:
node_modules/chalk/index.d.ts(403,16): error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.
Looks like they're using some constructs in the declaration that prohibit isolatedModules.

chalk/chalk#382

Apparently this was addressed as part of the latest 4.0.0 release. I'll see if I can remove the override now.

Edit: Reverted in d676360.

This reverts commit 40703df.

gziolo · 2020-04-09T04:41:34Z

It works like a charm:

gziolo · 2020-04-09T04:42:56Z

and when offline:

gziolo · 2020-04-09T04:53:01Z

bin/check-latest-npm.js

+
+It is required that you have the latest version of NPM installed in order to commit a change to the package-lock.json file.
+
+Run ${ yellow( 'npm install --global npm@latest' ) }, then try again.`


Should we expand the message with the recommendation to run npm install again after installing the latest version of npm?

Should we expand the message with the recommendation to run npm install again after installing the latest version of npm?

That's an excellent point, considering it's primarily the reason we're asking people to update 😅 (so that package-lock.json changes are accurate, which are adjusted only after npm install).

I can put together a small follow-up pull request.

aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 31, 2020

aduth requested review from gziolo and jorgefilipecosta March 31, 2020 23:17

aduth requested review from ajitbohra, nerrad and ntwb as code owners March 31, 2020 23:17

aduth mentioned this pull request Mar 31, 2020

Build Tooling: Reevaluate "Build Artifacts" NPM install script #16157

Closed

aduth commented Apr 1, 2020

View reviewed changes

bin/check-latest-npm.js Show resolved Hide resolved

aduth commented Apr 1, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

aduth mentioned this pull request Apr 3, 2020

First pass at I18N-specific ESLint rules #20555

Merged

6 tasks

aduth mentioned this pull request Apr 7, 2020

ESLint Plugin: Update ESLint and related deps to 6.8.x #21424

Merged

6 tasks

aduth added 6 commits April 7, 2020 17:28

Framework: Add package-lock precommit check for latest NPM

5b022ec

Framework: Rephrase messaging of install prompt

8dc4681

Framework: Expand on comment regarding Accept header

98d113b

Framework: Add chalk color in latest NPM check messaging

eca44a1

Framework: Override isolatedModules in bin TypeScript config

40703df

Framework: Provide messaging for fetch connectivity error

aac7243

aduth force-pushed the add/latest-npm-package-lock-check branch from 25b2cb5 to aac7243 Compare April 7, 2020 21:45

Framework: Bump chalk to 4.0.0

2612136

aduth requested review from epiqueras, noahtallen, noisysocks, talldan and youknowriad as code owners April 8, 2020 13:37

gziolo reviewed Apr 8, 2020

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

gziolo approved these changes Apr 8, 2020

View reviewed changes

Framework: Perform package-lock.json surgery

76b6501

To remove redundant chalk dependency entries (resolved instead from root)

Revert "Framework: Override isolatedModules in bin TypeScript config"

d676360

This reverts commit 40703df.

aduth merged commit 5572304 into master Apr 8, 2020

aduth deleted the add/latest-npm-package-lock-check branch April 8, 2020 20:17

github-actions bot added this to the Gutenberg 7.9 milestone Apr 8, 2020

gziolo reviewed Apr 9, 2020

View reviewed changes

aduth mentioned this pull request Apr 9, 2020

Build Tooling: Check Latest NPM: Advise additional npm install #21521

Merged

This was referenced Apr 30, 2020

Configure the navigation editor with correct __experimentalFetchLinkSuggestions #21873

Merged

[BUG] npm install or npm update sometimes trashes lockfile, setting resolved: false npm/cli#1138

Closed

aduth mentioned this pull request May 8, 2020

Build Tools: Validate package-lock.json for "resolved" errors #22237

Merged

aduth mentioned this pull request May 28, 2020

Add new package @wordpress/lazy-import for lazily installed packages #22684

Merged

ockham mentioned this pull request Jan 11, 2024

Workflow: Update to latest matching npm when publishing packages #57757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Add package-lock precommit check for latest NPM #21306

Framework: Add package-lock precommit check for latest NPM #21306

aduth commented Mar 31, 2020 •

edited

github-actions bot commented Mar 31, 2020 •

edited

gziolo commented Apr 1, 2020

aduth commented Apr 1, 2020

aduth commented Apr 1, 2020

sirreal commented Apr 1, 2020

aduth commented Apr 2, 2020

swissspidy commented Apr 3, 2020

aduth commented Apr 3, 2020

swissspidy commented Apr 3, 2020

aduth commented Apr 3, 2020 •

edited

aduth commented Apr 7, 2020

aduth commented Apr 8, 2020

gziolo commented Apr 8, 2020

gziolo left a comment

aduth commented Apr 8, 2020 •

edited

gziolo commented Apr 9, 2020

gziolo commented Apr 9, 2020

gziolo Apr 9, 2020

aduth Apr 9, 2020 •

edited


		It is required that you have the latest version of NPM installed in order to commit a change to the package-lock.json file.

		Run ${ yellow( 'npm install --global npm@latest' ) }, then try again.`

Framework: Add package-lock precommit check for latest NPM #21306

Framework: Add package-lock precommit check for latest NPM #21306

Conversation

aduth commented Mar 31, 2020 • edited

github-actions bot commented Mar 31, 2020 • edited

gziolo commented Apr 1, 2020

aduth commented Apr 1, 2020

aduth commented Apr 1, 2020

sirreal commented Apr 1, 2020

aduth commented Apr 2, 2020

swissspidy commented Apr 3, 2020

aduth commented Apr 3, 2020

swissspidy commented Apr 3, 2020

aduth commented Apr 3, 2020 • edited

aduth commented Apr 7, 2020

aduth commented Apr 8, 2020

gziolo commented Apr 8, 2020

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Apr 8, 2020 • edited

gziolo commented Apr 9, 2020

gziolo commented Apr 9, 2020

gziolo Apr 9, 2020

Choose a reason for hiding this comment

aduth Apr 9, 2020 • edited

Choose a reason for hiding this comment

aduth commented Mar 31, 2020 •

edited

github-actions bot commented Mar 31, 2020 •

edited

aduth commented Apr 3, 2020 •

edited

aduth commented Apr 8, 2020 •

edited

aduth Apr 9, 2020 •

edited