Add PR check to remove npm absolute paths #29
Conversation
robertbrignull
force-pushed
the
removeNPMAbsolutePaths
branch
from
8bedefd to
ec4d38a
Compare
| # Reinstall modules and then clean to remove absolute paths | ||
| # Use 'npm ci' instead of 'npm install' as this is intended to be reproducible | ||
| npm ci | ||
| npm run removeNPMAbsolutePaths |
There was a problem hiding this comment.
Should this have the --force flag?
The reason I added the --force flag to removeNPMAbsolutePaths is so that it rewrites all files to a canonical form. I was seeing some differences in ordering of properties of json objects when there were otherwise no changes. I hope by letting removeNPMAbsolutePaths decide this aspect we'll avoid any inconsistencies.
There was a problem hiding this comment.
No, that's a reference to the script defined in package.json
Line 10 in 51b42fc
I did it that way so then we can keep things consistent by only specifying the arguments there.
It is perhaps a bit confusing that the name is the same, unless you know what is going on. What do you think?
There was a problem hiding this comment.
I missed that. If it works I'm happy with it
|
Let's go with it. If we find it causes issues for some people then we'll have to rethink this and we can easily revert this PR. |
This PR aims to reduce the chance of us accidentally checking in a bad node_modules directory.
We add another PR check (and renames the workflow file as having four things in the name is a bit unwieldy) that runs
npm ciand alsoremoveNPMAbsolutePaths. Hopefully this will catch places where we modify a npm dependency but fail to update thenode_modulesdirectory, and will catch absolute paths being checked in.The danger is that running npm commands isn't very stable and we could end up with false positives because of differing behaviour between different npm versions or on different operating systems. The actions environment seems consistent with my local environment so I'd propose that we go for it and if this causes problems then we revert it.
The reason why I used
npm ciinstead ofnpm installis that I believe it's intended to be more stable. However the behaviour has changed a few times since it's introduction so it's all very unclear. npm is just not very good if you want reproducible results.The reason I added the
--forceflag toremoveNPMAbsolutePathsis so that it rewrites all files to a canonical form. I was seeing some differences in ordering of properties of json objects when there were otherwise no changes. I hope by lettingremoveNPMAbsolutePathsdecide this aspect we'll avoid any inconsistencies.