search

tradle / rn-nodeify

hack to allow react-native projects to use node core modules, and npm modules that use them
MIT License
615 stars 112 forks source link

fix: add a new line at the end of package.json to align with yarn and npm #84

Closed johnnytomcat closed 4 years ago

johnnytomcat commented 4 years ago

Related to https://github.com/yarnpkg/yarn/issues/2675

brysgo commented 4 years ago

Seems like a sensible thing to do. NPM actually uses this package: https://www.npmjs.com/package/stringify-package to write out package.json files. Maybe we should consider using it instead?

You can see the usage here: https://github.com/npm/cli/blob/latest/lib/install/save.js#L127

johnnytomcat commented 4 years ago

I just changed what these two changed https://github.com/npm/npm/blob/064d62c2f251cb5e9328f63228f711844b7a1787/lib/install/update-package-json.js#L15 https://github.com/facebook/create-react-app/pull/1510/files

brysgo commented 4 years ago

That is an old version of NPM - the repo moved.

Whoever changed CRA must have looked at the same thing you did.

I'm not sure whether we care about supporting different line endings, but stringify-package does: https://github.com/npm/stringify-package/blob/latest/index.js

Also, this would make it easier if they change the default formatting of the package json, since we would just have to update the package.

brysgo commented 4 years ago

Either way we end up doing it should fix the problem though so it looks good to me.

mvayngrib commented 4 years ago

@johnnytomcat sorry, i don't really understand the problem, can u explain what's broken without this?

brysgo commented 4 years ago

There are few things that I can think of:

  1. Linters configured to check for the newline
  2. Code formatters that automatically put it back
  3. Emacs users get very frustrated with no newline
  4. Because of this, the file will flip flops between newline and no newline causing merge conflicts.

I was on a team with an individual who for months would get so frustrated because he couldn't figure out who kept removing the newline from the package.json.

@johnnytomcat did I miss anything?

mvayngrib commented 4 years ago

@brysgo thanks for explaining. Just fyi, the package.json files are only written if rn-nodeify modifies them, which should only happen when you call rn-nodeify for the first time or after changing its call arguments. Still, for those cases, this should help, thanks @johnnytomcat :)

johnnytomcat commented 4 years ago

Thanks! @brysgo you nailed the issue we were running into. This is the exact reason for this change.

johnnytomcat commented 4 years ago

@mvayngrib Can we make a release with this in it?

mvayngrib commented 4 years ago

@johnnytomcat yep, released immediately after merging, see version 10.2.0

johnnytomcat commented 4 years ago

Thank you!