I would go so far as to say that this is UBN!

Change 456028 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Update package.json to ensure assets get built on every commit

https://gerrit.wikimedia.org/r/456028

In addition to @Jdlrobson's patch, @Niedzielski had a similar WIP here
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Popups/+/454566/

In the comments, Stephen mentioned off-hand that this might be an issue with different NPM versions.
I tested this today, and it appears that it is!

Using nvm, I switched to Node V6 (lts/boron -> v6.14.4), deleted my node_modules folder, did an npm install, and ran npm -s run build
The output was different than when I ran the same process with Node V9.

These might be differences in the minifier. Regardless, I don't think we can change this behaviour unless we run an older version of Node locally, or CI runs a newer version.

Also of note, the current script runs git diff -q resources/dist I couldn't find any references to -q in the git docs, only --quiet.

Change 456028 abandoned by Jdlrobson:
Update package.json to ensure assets get built on every commit

https://gerrit.wikimedia.org/r/456028

Change 456028 restored by Jdlrobson:
Update package.json to ensure assets get built on every commit

https://gerrit.wikimedia.org/r/456028

Change 456028 abandoned by Jdlrobson:
Update package.json to ensure assets get built on every commit

https://gerrit.wikimedia.org/r/456028

Change 456028 restored by Jdlrobson:
Update package.json to ensure assets get built on every commit

https://gerrit.wikimedia.org/r/456028

Blocked by T203522

Change 456028 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Update package.json to ensure assets get built on every commit

https://gerrit.wikimedia.org/r/456028

Change 459594 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Do not merge: test Node v10.9.0 built assets

https://gerrit.wikimedia.org/r/459594

Change 459597 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Do not merge: test unbuilt inputs

https://gerrit.wikimedia.org/r/459597

Change 459594 abandoned by Niedzielski:
Do not merge: test Node v10.9.0 built assets

Reason:
Success! A build product difference causes the patch tests to fail:

18:18:26 Binary files a/resources/dist/index.js and b/resources/dist/index.js differ
18:18:26 diff --git a/resources/dist/index.js.json b/resources/dist/index.js.json

https://gerrit.wikimedia.org/r/459594

Change 459597 abandoned by Niedzielski:
Do not merge: test unbuilt inputs

Reason:
Abandon! The build outputs differ:

18:30:25 diff --git a/resources/dist/index.js b/resources/dist/index.js
18:30:25 index 80895dd..32c5eac 100644
18:30:25 Binary files a/resources/dist/index.js and b/resources/dist/index.js differ
18:30:25 diff --git a/resources/dist/index.js.json b/resources/dist/index.js.json
18:30:25 index ab577b5..151463c 100644
18:30:25 Binary files a/resources/dist/index.js.json and b/resources/dist/index.js.json differ

https://gerrit.wikimedia.org/r/459597

In T203591#4574473, @zeljkofilipin wrote:

I'm not sure what the problem is. Are you saying I can't make a commit to Popups unless I have Node v6.11.0? If that is the case, it should be documented in ALL CAPS in the readme. 😉

you can make a commit, but you if you run npm run build the version is important.
It seems the error message is not being broadcast well enough.
I don't think adding to the README makes sense, as we already make the node version clear inside package.json and .nvmrc as well as the script that fails.
We should probably update the README to ensure that the correct version of Node is being used and that we recommend using nvm.

For what it's worth, @zeljkofilipin, I think this problem will go away in time. Once NPM v5 is deployed in CI, lockfiles will be honored and hopefully we won't have build differences across newer Node.js / NPM versions.

In T202748#4574859, @Jdlrobson wrote:

you can make a commit, but you if you run npm run build the version is important.

No, I could not make the commit. See T203591#4562169. Popups has a precommit git hook and it fails on my machine, aborting the commit. All I wanted to do is add a dev dependency to package.json (one line change), but I was not even able to make a commit.

It seems the error message is not being broadcast well enough.
I don't think adding to the README makes sense, as we already make the node version clear inside package.json and .nvmrc as well as the script that fails.
We should probably update the README to ensure that the correct version of Node is being used and that we recommend using nvm.

I might just be more ignorant than an average contributor, but I've sent minor patches to many repositories and I can't remember ever being unable to make a commit.

I was not aware that node version can be specified in package.json and I have never used nvm, so I've ignored .nvmrc. Now that I know that, it's obvious. I did check readme file but I could not find any special requirements for the repo.

In T202748#4574904, @Niedzielski wrote:

For what it's worth, @zeljkofilipin, I think this problem will go away in time. Once NPM v5 is deployed in CI, lockfiles will be honored and hopefully we won't have build differences across newer Node.js / NPM versions.

All problems will go away in time. 😄

The problem with the commit is the pre-commit hook (which runs the npm run build command) is failing. You should be able to disable it to not run into this problem.

Is there a task tracking NPM v5 being used in CI? I want to work out what the trade-offs here are for making this better or waiting for that. It's clearly not ideal right now and is annoying for us too.