Differential D25004
Update .gitignore to account for package-lock.json Authored by speck on Jun 18 2021, 19:08.
Details
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. 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
Event TimelineComment Actions 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 ] Comment Actions 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
Comment Actions 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. Comment Actions Just confirmed on another instance:
Comment Actions 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. Comment Actions Yea it probably won’t have an effect until they run npm install/ci. We could create those files and include. I’ll update Comment Actions 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. Comment Actions What happens if
Will this cause a conflict? - I feel like it will..? Comment Actions Will it cause a conflict or ask the user to commit or stash untracked changes? But yeahhhhhh, will need some human intervention... Comment Actions I created T15013: Better handling of node/npm installation for Aphlict for further discussion Comment Actions 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. Comment Actions Fair point on being precise with the terminology. 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. Comment Actions What do you think of this instead # Local NPM files created during Aphlict installation Comment Actions 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. Comment Actions 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 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 |