Page MenuHomePhorge

Update .gitignore to account for package-lock.json
AbandonedPublic

Authored by speck on Jun 18 2021, 19:08.
Referenced Files
Unknown Object (File)
Wed, Sep 28, 13:15
Unknown Object (File)
Wed, Sep 28, 13:15
Unknown Object (File)
Wed, Sep 28, 13:15
Unknown Object (File)
Wed, Sep 28, 13:15
Unknown Object (File)
Tue, Sep 27, 03:00
Unknown Object (File)
Tue, Sep 20, 15:04
Unknown Object (File)
Tue, Sep 20, 15:04
Unknown Object (File)
Tue, Sep 20, 15:03
Subscribers

Details

Summary

The documentation for setting up Aphlict instructs running npm install ws in the support/aphlict/server/ directory. At the time Aphlict was developed the node/npm tools did not create package-lock.json files.

This updates the .gitignore file to also ignore the package-lock.json file created from the installation.

Test Plan

I checked this on we.phorge.it under /var/wwww/phorge to verify this modification to .gitignore properly ignores the package-lock.json file, as it's also present on the server.

Diff Detail

Repository
rP Phorge
Branch
aphlict
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10
Build 10: arc lint + arc unit

Event Timeline

speck requested review of this revision.Jun 18 2021, 19:08
speck created this revision.
This revision is now accepted and ready to land.Jun 18 2021, 19:14

Do we actually want to be version controlling this? That's the recommended approach for Node projects, and given how hilariously awful dependency management is with npm, it might simplify support if we say "Aphlict runs with this specific version of websockets and its dependencies".

[Edit: Ref, e.g., https://stackoverflow.com/a/44210813 ]

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

  1. Run npm uninstall
  2. Delete package-lock.json
  3. Upgrade
  4. Run npm ci which should follow the package-lock.json definitions

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.

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

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.

speck planned changes to this revision.Jun 18 2021, 19:48

Yea it probably won’t have an effect until they run npm install/ci. We could create those files and include. I’ll update

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.

What happens if

  • User A has a local, untracked /support/aphlict/server/package-lock.json
  • We update Phorge and start tracking this file
  • User A updates their local install with a git pull

Will this cause a conflict? - I feel like it will..?

Will it cause a conflict or ask the user to commit or stash untracked changes? But yeahhhhhh, will need some human intervention...

This revision is now accepted and ready to land.Jun 18 2021, 21:19

Will this cause a conflict? - I feel like it will..?

Not really a conflict in the normal sense, but git will refuse the pull.

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.

Fair point on being precise with the terminology.

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.

If that's the case, you're already going off-book WRT the Diviner docs, right? The Notifications User Guide only extends to npm install, and I don't think we want to cover every possible installation path for every possible system.

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.

What do you think of this instead

# Local NPM files created during Aphlict installation

Updating comment per discussion

speck requested review of this revision.Jun 19 2021, 04:48

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.

Without package-lock.json, it is not possible to deploy a consistent set of dependencies resolution - they might change any time any one publishes a new package, which creates a lot of problems for reproducibility.

Without package-lock.json, it is not possible to deploy a consistent set of dependencies resolution - they might change any time any one publishes a new package, which creates a lot of problems for reproducibility.

Further to this, if we did go this route - Then we should probably add a package.json that specifies what is being installed. And then update the docs
https://secure.phabricator.com/book/phabricator/article/notifications/

You will also need to install the ws module for Node. This needs to be installed into the notification server directory:

phabricator/ $ cd support/aphlict/server/
phabricator/support/aphlict/server/ $ npm install ws
Once Node.js and the ws module are installed, you're ready to start the server.
Simplified Install Instruction if we add in package.json
phabricator/ $ cd support/aphlict/server/
phabricator/support/aphlict/server/ $ npm install

[...]

You are correct. I moved on with this in D25006