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

Format and standardize package.json files #1512

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

friederbluemle
Copy link
Member

Related: #1473 #1506 #1507

Part of #1511

With this, all 18 primary package.json files in the repo follow a common and standardized formatting and key order.

Some significant simplifications and line savings were achieved by using simple strings instead of full objects for contributors. All dependencies were sorted alphabetically.

This PR does not change/update any dependencies or add/remove any fields.

While there is no single authoritative field order for package.json files, several projects exist that aim to provide a "opinionated standard", e.g. sort-package-json, fixpack, prettier-package-json. It's important to point out that all three projects are incompatible with each other, meaning when the default configuration is applied to the same package.json file, it will continuously cause reformatting.

Lacking a "globally accepted standard", the key order chosen in this PR was derived from the following (from highest to lowest priority):

  1. The ordering of a default package.json as generated by npm/Yarn
  2. The official documentation of package.json by npmjs.org
  3. As much as possible, keeping common patterns set by the three tools mentioned above
  4. When possible, keeping common patterns in this repo to minimize the required line changes

In particular the first item can be considered as the only "true" standard. The official docs by npmjs.org are missing several possible keys, and are not fully in sync with what is generated by npm/Yarn.

Tool config details

sort-package-json

Does not allow configurations, so it can not be used as the final formatter.

prettier-package-json

A modified default config has been pushed here:
https://github.com/friederbluemle/prettier-package-json/blob/fb-custom/src/defaultOptions.js

The order can also be passed on command line:

prettier-package-json --key-order \$schema,name,version,private,description,main,browser,types,files,bin,scripts,repository,keywords,author,contributors,license,bugs,homepage,man,directories,config,dependencies,devDependencies,peerDependencies,bundledDependencies,optionalDependencies,engines,os,cpu,publishConfig,workspaces

Note: Currently, prettier-package-json will also reorder scripts in a non-alphabetical way. See cameronhunter/prettier-package-json#26 for additional details. There will be modified lines when using it in this repo.

fixpack

A .fixpackrc file with the custom order can be found here: https://github.com/friederbluemle/misc/blob/master/.fixpackrc
With this file, fixpack should fully pass all newly formatted package.json files.

Copy link
Member

@belemaire belemaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this 👍
I'll also add prettier-plugin-package to the list, which has the advantage of being a prettier plugin, so automatically used by Prettier if installed. It is also opinionated, with key sorting rules based on conventions used by reference developer @sindresorhus But it is strongly opinionated in the sense that unlike other packages referenced, it does not allow any kind of configuration to alter formatting/sorting rules.

I don't really care about opinionated rules, especially if there is no common agreed upon standard, as long as the code base is consistent and remains consistent with these rules.

I am O.K to have this PR merged, but even if we document 'our' standard, we cannot expect all contributors to have it read, nor can we expect our core team to know about them and make sure to verify every PR modifying some package.json to manually check for compliance with our formatting rules.

So to ensure that formatting of these files remain consistent, we need to have automated checks/formatter in place for these files. Ideally it should have been part of this PR (and that's where I don't really mind using some opinionated tool as long as it keeps the automated setup on the simple side of things).

Let's get this one merged, as it improves formatting of these files and consistency across code base, but we need to come sooner than later with another PR to automate checking / formatting of these files (where we might have to do some compromise on this current formatting structure -or not-).

@friederbluemle
Copy link
Member Author

Thanks for the detailed review and comment @belemaire - And thanks for the link to prettier-plugin-package

I agree with what you said, just wanted to add a couple of things:

I also don't care about opinionated rules - especially if they're coming from a non-authoritative source, are not used by the "vast majority" of users, and/or are conflicting with what is generated by the primary tools (in this case npm/Yarn). Sometimes, it's immediately clear what that authority is (e.g. gofmt for golang), in other cases such as the key order of package.json, no real uniform standard exists (as mentioned). For such gray areas, it's difficult to come up with objective guidelines... As explained in detail in the PR description, the key order was derived in the least subjective way possible based on several existing sources. If there are other tools and/or standards that I missed, please mention them here and we can consider them as well. I did try prettier-plugin-package, and - as you mentioned - it does not allow customization (just like sort-package-json). Unfortunately applying the tool to a default package.json generated by npm init --yes does not result in a 0-line difference.

I agree 100% that these formatting inconsistencies should ideally be caught at PR stage - Something I also mentioned in #1454 and #1511. Of course, not every repo starts from commit number one with a fully loaded CI that detects every minor inconsistency possible.

It's important to make incremental improvements.

The primary purpose of this PR here is remove the existing inconsistencies we have in the repo today. Automated checks can and should be added as well, but in order to (1) Keep the commits as focused as possible and (2) Avoid broken commits on the master branch, I'm asking for this to be done at a later point. Even without automated checks, it is very unlikely that we will immediately see a reintroduction of inconsistencies. The reasons are: There have been very little changes in the package.json ordering for a very long time, and most importantly: Adding dependencies (the most common modification) using npm add or yarn add results in the new entries being inserted in the correct alphanumerically sorted location by the tool automatically.

@belemaire
Copy link
Member

👍 Thanks for reply rather than immediate ninja merge 😉

There have been very little changes in the package.json ordering for a very long time, and most importantly: Adding dependencies (the most common modification) using npm add or yarn add results in the new entries being inserted in the correct alphanumerically sorted location by the tool automatically.

Completely agrees with that. Regarding keys ordering, I don't think anyone is going to consciously modify the order of some of the keys through a PR (unless they have an IDE that applies some auto-formatting rules on save and they don't notice the changes upon PR submission), and the chance of new keys being introduced is very low (as you say very little changes has been made to these files apart from dependencies alterations) and if that's the case it should be a red flag when reviewing the PR. For the ordering of dependencies though, I sometimes add them manually, but I should just pay attention when doing so, or change my habits ;) But apparently even if using yarn/npm it might result in ordering difference (see this issue reported) not sure if the CLIs that are being talked are yarn v.s npm, still 😓

Anyway, given the low modification rate of these files, it's indeed not necessary to have auto formatting available immediately. For other files such as source files, modified much more frequently, it would be a waste to reformat them without also coming up with an automated formatting or formatting checks coming as part of the PR as they would diverge from formatting rules very quickly, but that's not really the case here.

Let's get this one merged 😄 🚀

@friederbluemle
Copy link
Member Author

Sounds good! 👍

@friederbluemle friederbluemle merged commit cad22e7 into electrode-io:master Mar 6, 2020
@friederbluemle friederbluemle deleted the package-json branch March 6, 2020 23:32
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

Successfully merging this pull request may close these issues.

2 participants