Jun 29 2021
Jun 25 2021
D25012 solves it, but I wonder if we should just exclude it from the "type": "text" linters
ahh, there's a global setting for revisions - differential.generated-paths.
I was thinking of ArcanistGeneratedLinter.php, but it looks like the only way to mark a file is by adding @generated in it, and I think we can't do that?
I thought there was another way, because this also applies to revisions (it folds the file by default).
Jun 21 2021
There might be a way to explicitly define it as generated, which (used to) exclude it from lint.
I see that this has been added to the changelog already here: https://we.phorge.it/w/changelog/2021.25/ . Fantastic!
I landed the code, but leaving the task open because we need to add a release note whenever we know where they go.
Jun 20 2021
Jun 19 2021
Abandoning for D25006
While I'm not super familiar with this setup, it is customary in the node space to commit package-lock.json int he repository. If it is intended to put stuff in there which are not to be committed at all - the folder is a placeholder - then it is best to ignore the whole folder. If committing what's in there is intended, then package-lock.json should be included.
Updating comment per discussion
Jun 18 2021
Fair point on being precise with the terminology.
I have some concerns regarding the new comment, as these files don't really belong to aphlict, but are npm npm artifcats. If node-ws is installed via e.g. the system package manager (which is probably the better idea anyway), these files/folders won't be created.
I created T15013: Better handling of node/npm installation for Aphlict for further discussion
Will it cause a conflict or ask the user to commit or stash untracked changes? But yeahhhhhh, will need some human intervention...
What happens if
The problem with package-lock.json is, that it either generates noise in the working copy (it just changes its content a lot of times during normal operation). And it needs an update every time a dependency (even indirect) got an security related update because production installs will otherwise not pull the updated dependency in.
Yea it probably won’t have an effect until they run npm install/ci. We could create those files and include. I’ll update
AFAIK package-lock.json is only used by npm itself, when deciding on which version to download. If no package-lock.json exists the version are taken to match package.json.
Just confirmed on another instance:
- Had an existing package-lock.json pinning ws to version 7.0.0
- Manually updated package-lock.json to pin to version 7.5.0 (did not actually update installed version of ws)
- Cycled Aphlict service
- Sent a test notification
- Received Web + Desktop popups
It _shouldn't_ mess up anything existing, I don't think. It's been a few months since I did a ton with JS so might be forgetting something obvious here, but if anyone already has Aphlict up and running, I'm pretty sure their existing install will be unimpacted by the presence/absense of package-lock.json. If they want to manually update their npm packages, then they might need the additional steps, but pretty sure it won't be disruptive outside of that.
I was thinking about having it version controlled and I do think that would be a good idea at some point. If we do that now I think that might mess up installations which happen to be running different versions of ws, or the upgrade path would require some additional steps. I think it would be something like
- Run npm uninstall
- Delete package-lock.json
- Run npm ci which should follow the package-lock.json definitions