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

package.json files are not POSIX compliant #156

Closed
pixelzoom opened this issue Dec 11, 2019 · 19 comments
Closed

package.json files are not POSIX compliant #156

pixelzoom opened this issue Dec 11, 2019 · 19 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 11, 2019

It looks like PhET's package.json files do not conform to the POSIX specification, and are therefore being modified by newer versions of npm.

I'm running npm 6.1.21.. When I run npm install in any repo directory, it changes package.json, adding an extra blank line at the end. It does this each and every time. If I generate a patch using WebStorm for the modified package.json, I see this:

Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- package.json	(revision 546e35c77b0ec3756edd646f49e4bc85a3443074)
+++ package.json	(date 1576087807004)
@@ -38,4 +38,4 @@
       "alert": false
     }
   }
-}
\ No newline at end of file
+}

Note the "\ No newline at end of file".

Apparently PhET's package.json files don't conform to the POSIX standard. According to StackOverflow and opengroup.org, the definition of a "Line" in a file is:

3.206 Line
A sequence of zero or more non- characters plus a terminating character.

So I'm guessing that npm is writing a POSIX compliant package.json, and adding a newline after the final '}'.

Should we update all package.json files to be POSIX compliant? Eagerly? Lazily?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 11, 2019

This same issue (npm installmodifying package.json) was reported in npm/npm#18545.

More at https://stackoverflow.com/questions/50622958/how-to-re-write-json-to-a-file-so-as-not-to-get-no-newline-at-end-of-file-di

Also wondering if there's an issue with how UNIX vs Windows define a line's "terminating character". https://stackoverflow.com/questions/52604191/how-to-prevent-npm-install-change-package-json?noredirect=1&lq=1

@jessegreenberg
Copy link
Contributor

So this is a requirement for all files (not just package.json)? I have a Sublime Text setting

    // Set to true to ensure the last line of the file ends in a newline
    // character when saving
    "ensure_newline_at_eof_on_save": false,

and can set to true if this is required.

@samreid
Copy link
Member

samreid commented Dec 12, 2019

When I run npm install I do not see a change in package.json.

~/github/natural-selection$ npm --version
6.12.0
~/github/natural-selection$ npm install
audited 179 packages in 1.219s
found 0 vulnerabilities



   ╭────────────────────────────────────────────────────────────────╮
   │                                                                │
   │      New minor version of npm available! 6.12.0 → 6.13.4       │
   │   Changelog: https://github.com/npm/cli/releases/tag/v6.13.4   │
   │               Run npm install -g npm to update!                │
   │                                                                │
   ╰────────────────────────────────────────────────────────────────╯

~/github/natural-selection$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

@samreid
Copy link
Member

samreid commented Dec 12, 2019

Same result (no change to package.json) after I updated to npm 6.13.4. I wonder if we have different settings?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 12, 2019

@samreid your OS and version?

My npm settings are....

$ cat $HOME/.npmrc
package-lock=false

I ran npm install -g npm to update to 6.13.4. I'm still seeing package.json modified when I run npm install. E.g.:

% cd natural-selection
% npm install
git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   package.json

no changes added to commit (use "git add" and/or "git commit -a")
%

@jessegreenberg
Copy link
Contributor

I have Windows 10, npm version 6.13.4. When running npm install in natural-selection or others I don't see any changes to package.json.

@samreid
Copy link
Member

samreid commented Dec 12, 2019

@samreid your OS and version?

macOS 10.15.1

~/github$ cat $HOME/.npmrc
save=false

@ghost
Copy link

ghost commented Dec 18, 2019

@pixelzoom Just chiming in to note that contrary to popular belief, POSIX does not mandate files to end with a trailing newline, so your package.json is indeed POSIX-compliant. @Alexsey has summarized this here.

The more technical answer is that in POSIX standard there is a 3.206 Line definition that tells what the line is. It has nothing about EOF - it is about EOL. But also there is a 3.195 Incomplete Line definition. That defines a line at the EOF that has no termination symbol at the EOL. So files with and without line termination at the EOF from the perspective of the standard are completely the same files. The difference is only in lines, and this difference nowadays is just a stylistic

@pixelzoom
Copy link
Contributor Author

Thanks @Kdex!

@jonathanolson
Copy link
Contributor

I've generally included a newline at the end, and I'm fine with whatever others decide.

@pixelzoom
Copy link
Contributor Author

Another interesting side-effect.... Because PhET package.json files don't end with a newline, inspecting (e.g.) natural-selection/package.json in an editor shows that it contains 25 lines.

But some shell tools have a problem with this and don't read the last line. Examples...

% wc -l natural-selection/package.json
24

This script to read each line of the file only reads the first 24 lines:

#!/bin/bash
input="natural-selection/package.json"
while IFS= read -r line
do
  echo "$line"
done < "$input"

This seems like a potential problem for PhET's toolchain.

@ghost
Copy link

ghost commented Jan 2, 2020

@pixelzoom I don't know enough about this project to be heavily invested in this issue, though I'd still like to clarify the probable misconception of wc -l counting the lines in a file and also give advice on how to handle both \n-terminated as well as non-\n-terminated files.

The man page of wc is relatively clear that it counts the newlines (i.e. the newline characters) in a file, so your result makes total sense.

NAME
       wc - print newline, word, and byte counts for each file

If you want to count the lines (as opposed to newline characters) in a file instead, you might want to count the beginnings of a line as opposed to endings:

$ grep -c ^ natural-selection/package.json

As for your second example where you iterate through a file using while and IFS= to preserve line indentation, I'd recommend some extra robustness by instructing your loop body to execute whenever a line could be read, despite read having prematurely detected the end of the file.

Basically, all that's needed is || [ -n "$line" ] and you've got support for both \n-terminated and non-\n-terminated EOFs.

 #!/bin/bash
 input="natural-selection/package.json"
-while IFS= read -r line; do
+while IFS= read -r line || [ -n "$line" ]; do
        echo "$line"
 done < "$input"

No matter which EOF flavor you end up picking, I'd definitely recommend making your scripts work with both, as it allows you (and the maintainers after you) to change personal preferences without touching the toolchain later on.

@pixelzoom
Copy link
Contributor Author

Thanks (again!) @Kdex. The clarifications are much appreciated.

Any thoughts on why npm install is adding a newline to package.json? And why that might be happening only to me? Other members of our team with identical macOS and npm versions are not experiencing this.

@ghost
Copy link

ghost commented Jan 3, 2020

@pixelzoom Sure thing!

The newline issue is a long-standing bug in npm and affects many subcommands such as npm version or even npm install. There have been numerous issues about this, one of which can be viewed here. Though I'm not sure why your other team members aren't affected.

The reason why this happens is that npm will always try to rewrite your entire configuration whenever you run certain npm commands, even if your dependencies haven't changed.

It used to be much worse; some time ago, npm subcommands would rewrite your package.json and package-lock.json indentation to use two spaces whenever they were rewritten. Nowadays, it will respect your indentation and finally allow you to use tabs.

Unfortunately, the repository from above has been archived. I can remember somebody having made a PR to fix npm's wrong EOF behavior, but if I remember correctly, it had been rejected because of said archival, and the author had been asked to migrate his PR to the new repository. Their new repository is here and doesn't currently seem to have any issues open with the word "newline" in them, so it is likely that they currently don't have this issue on their radar.

Nevertheless, you can currently avoid this bug by running npm install --no-save which prevents your files from being rewritten.

@pixelzoom
Copy link
Contributor Author

Awesome, thanks @Kdex.

@pixelzoom
Copy link
Contributor Author

npm config set save false resolved this issue for me, and that step is noted in the PhET Developer Overview. So I'm going to close this issue.

@mattpen
Copy link
Contributor

mattpen commented Sep 9, 2021

This work-around did not work for me, nor did the --no-save option. This is preventing me from running rc/production tests.

[~/phet/git/chains]$ npm config set save false
[~/phet/git/chains]$ npm config get save    
false
[~/phet/git/chains]$ git status | grep modified
[~/phet/git/chains]$ npm update > /dev/null
[~/phet/git/chains]$ git status | grep modified
	modified:   package.json
[~/phet/git/chains]$ git stash                 
Saved working directory and index state WIP on 1.34: 61afb3f Bumping version to 1.34.0
[~/phet/git/chains]$ git status | grep modified
[~/phet/git/chains]$ npm update --no-save > /dev/null
[~/phet/git/chains]$ git status | grep modified      
	modified:   package.json
[~/phet/git/chains]$ git stash                       
Saved working directory and index state WIP on 1.34: 61afb3f Bumping version to 1.34.0
[~/phet/git/chains]$ git status | grep modified
[~/phet/git/chains]$ npm update --save=false > /dev/null
[~/phet/git/chains]$ git status | grep modified         
	modified:   package.json
[~/phet/git/chains]$ npm -v
7.7.6

@mattpen
Copy link
Contributor

mattpen commented Sep 9, 2021

I opened an issue in the new npm repo back in April, but it hasn't received any traction: npm/cli#3044

@mattpen
Copy link
Contributor

mattpen commented Sep 9, 2021

This seems to be related to Node 15, which is no longer supported. Using Node 14 or 16 solved the problem, so I'm going to leave this closed.

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

No branches or pull requests

5 participants