Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src/package.json file is modified when starting etherpad #3396

Closed
RalfJung opened this issue May 20, 2018 · 5 comments
Closed

src/package.json file is modified when starting etherpad #3396

RalfJung opened this issue May 20, 2018 · 5 comments
Milestone

Comments

@RalfJung
Copy link

Steps to reproduce

I am launching etherpad directly from the clone source tree by starting bin/run.sh, with NODE_ENV=production in the environment.

Actual behavior

After doing that, git status tells me that src/package.json has been modified. From the looks of it, something parsed and re-formatted the json in there.

Expected behavior

Launching an application should not modify source files that are tracked by the VCS. Hence I would expect git status to be clean after launching etherpad.

@muxator
Copy link
Contributor

muxator commented Jul 14, 2018

@RalfJung, that's true. After googling around, it appears to be standard (albeit surprising) behaviour on npm side. The command that triggers reformatting is npm install --loglevel warn.

The npm developers do not seem to be willing to change that (see npm/npm#3299), so probably in order to mitigate this issue, a different approach will be needed.

Personally, I do not see a reason for which a config file needs to be opened RW at all.

@RalfJung
Copy link
Author

npm/npm#3299 seems to be about "npm shorthands for updating your JSON". Is etherpad using those when being started, or how is that related?

@muxator
Copy link
Contributor

muxator commented Jul 14, 2018

You are right, @RalfJung, wrong link.

It turns out, at some point of its development npm default config for save was changed to true. This means that if you have a newer npm version and you use the default config, npm will rewrite package.json on installation.

This makes no sense for installDeps.sh, because its only (big) side effect should be to actually install dependencies according to a configuration file, and not to modify it.

I have made some tests, and to fix this it is possible to add --no-save to the npm command line in installDeps.sh. No concerns about backwards compatiblity with oldish npm version, as they seem to support --no-XXX style flags even if not documented.

@RalfJung
Copy link
Author

Thanks :)

@muxator
Copy link
Contributor

muxator commented Oct 31, 2019

I am going to revert my decision and put back --save in installDeps.sh. For the motivations, see #3659.

In the meantime, the committed version of package.json was reformatted (see c4918ef and subsequent commits), so at least any change on that file will be easily identifiable looking at the diff.

muxator added a commit that referenced this issue Nov 1, 2019
…e repo.

This change reverts c4918ef, and basically negates what was done for #3396,
but aligns better with current practices in the nodejs ecosystem.

Pragmatically speaking, this will allow users, if they want, to use
npm-force-resolutions (https://github.com/rogeriochaves/npm-force-resolutions)
to manually fix security vulnerabilities.
We had a problem for that (see #3598), and - given the fragmented nature of
the nodejs ecosystem - it is reasonable to expect more issues like that one,
so it's better to be prepared.

Closes #3659.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants