Repository: atlassian/changesets
Pull request #395 (2020-06-17 23:10:28)

fix: use defaults to format package.json
prettier formatting of package.json files now defaults to using json-stringify.
Specifying `json` explicitly was causing some inconsistencies

atlassian-cla-bot[bot]: Thank you for your submission! Like many open source projects, we ask that you sign our <a href=https://opensource.atlassian.com/cla>CLA (Contributor License Agreement)</a> before we can accept your contribution.
      <strong>If your email is listed below</strong>, please ensure that you sign the CLA with the <strong>same email address</strong>.</br>
      <strong>The following users still need to sign our CLA:</strong></br>❌jonathanmorley
<p><sub>Already signed the CLA? To re-check, try refreshing the page.</sub></p>
changeset-bot[bot]: ###  💥  No Changeset

Latest commit: 8a5ae4d26d5b07e96f8555a042748ee81b7fc406

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

**If these changes should be published to npm, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/jonathanmorley/changesets/new/patch-2?filename=.changeset/loud-taxis-joke.md&value=---%0A%22%40changesets%2Fapply-release-plan%22%3A%20patch%0A---%0A%0Afix%3A%20use%20defaults%20to%20format%20package.json%0A)


Andarist: Taken from Prettier docs:
> "json-stringify" (same parser as "json", but outputs like JSON.stringify) First available in v1.13.0

Do you actually know what's the actual difference? 🤔 Do you know in which version the default has changed?
jonathanmorley: It appears that `json-stringify` was used for `package.json` files since it was introduced (See the `filenames` property under https://github.com/prettier/prettier/commit/1062f2a8898b9e1ba00de1a535797ce9b22d102a#diff-ef736ec1de130f41ef22f8e5c31df9f9)

The only difference i have seen is when I have a small objects and arrays. Specifically, having

```
{
  "files": [
    "lib"
  ]
}
```

in a package.json will be valid according to prettier defaults and prettier with the `json-stringify` parser.

However, prettier with the `json` parser (and by extension changesets) will change it to

```
{ "files": ["lib"] }
```

This was reproducible across prettier 1.18.2 (the lowest version that changesets is pinned at), and prettier 2.0.5 (latest)
Andarist: I'm wondering if maybe we shouldn't just use a custom modifier for this, for example using this: https://github.com/microsoft/node-jsonc-parser . By converting input json and getting locations from the AST we could just manipulate the input json without any formatting changes and this is basically what we are after here as we don't care about the formatting of a file.
jonathanmorley: I suspect that https://github.com/atlassian/changesets/commit/90f8bc95d8b47628bc07404084e0400c387dc9cc was introduced as a partial workaround to this issue.

Given:

```
09:38:23 AM ❯ cat --plain tmp/package.json
{
  "files": ["lib"],
  "files2": ["longer file"],
  "files3": ["even longer filename"]
}
```

Compare

```09:38:30 AM ❯ npx prettier tmp/package.json --parser json --print-width 20
npx: installed 1 in 1.331s
{
  "files": ["lib"],
  "files2": [
    "longer file"
  ],
  "files3": [
    "even longer filename"
  ]
}```

with 

```
09:38:52 AM ❯ npx prettier tmp/package.json --parser json-stringify
npx: installed 1 in 0.968s
{
  "files": [
    "lib"
  ],
  "files2": [
    "longer file"
  ],
  "files3": [
    "even longer filename"
  ]
}```
jonathanmorley: I think the custom modifier would be preferable, but would you want it instead of this PR? or as a follow up?
Andarist: Personally I would prefer repurposing this PR as the current solution only moves a problem (and not just in theory) because one can easily have custom prettier settings for JSON files (or no prettier at all) and this would in turn change formatting for those people.

This would increase the overall complexity of the solution, so I would prefer receiving a green light for this from @mitchellhamilton or @Noviny before proceeding further. It would be a shame to put a considerable effort into this to only have the work dismissed later.
jonathanmorley: One point of contention is that for people using the default prettier settings, changesets is _already_ broken, since it overrides those settings with its own
Andarist: > Custom prettier settings should still be respected.

Right, if possible - yes, definitely. As the changes that have to be done to the package.json are minimal though I feel like the easiest solution is to actually avoid reading any configs, or providing custom configs or things like that. Just update the JSON in place - probably could be done with RegExps, but I don't think they are ultimately best for this kind of a job, especially that they are super hard to maintain. Hence my proposal to just parse JSON to the AST, traverse it to get input locations and manipulate the input string based on this information.
maraisr: As @Andarist has said:
> I'm wondering if maybe we shouldn't just use a custom modifier for this, for example using this: https://github.com/microsoft/node-jsonc-parser . By converting input json and getting locations from the AST we could just manipulate the input json without any formatting changes and this is basically what we are after here as we don't care about the formatting of a file.

I believe this to be a better way of doing it. This way there is no formatting bias to those that don't use prettier. I guess the only concern here is, if you are using prettier—could a version bump inline replace using the AST no longer render the file validly with prettier? I doubt it.
jonathanmorley: I wasn't able to reproduce the tabs/spaces issue from #396. I would welcome the addition of tests around that, especially if we can show that this change fixes it
mitchellhamilton: Sorry I missed this earlier but adding jsonc-parser seems like a lot of complexity for no big gain. Let's use something like this to write the package.jsons given that package managers already use `JSON.stringify` when writing

```tsx
import * as fs from "fs-extra";
import detectIndent from "detect-indent";

let pkgRaw = await fs.readFile(packageJsonPath, "utf-8");
let indent = detectIndent(pkgRaw).indent || "  ";
await fs.writeFile(
  packageJsonPath,
  JSON.stringify(pkgJsonContent, null, indent) +
    (pkgRaw.endsWith("\n") ? "\n" : "")
);
```
Andarist: @mitchellhamilton I'm afraid that this doesn't solve the problem at all or would only push the problem towards other cases. The proposed solution only tries to preserve the original indentation, but the problem here is not about indentation being changed, but rather formatting of particular values, mainly things like this:
```json
{
  "key": ["val1"]
}
```
This array is being sometimes kept in a single line and sometimes spread into multiple lines.
mitchellhamilton: > This array is being sometimes kept in a single line and sometimes spread into multiple lines.

When is it kept in a single line(except if someone manually changes it which I'm not really concerned about)? Given that prettier formats package.json's with JSON.stringify now(or rather something that is functionally equivalent) and package managers use JSON.stringify, I don't see when it would happen?
Andarist: It can be kept in a single line when somebody is using prettier's `"json"` parser - which seems like an easy "mistake" to make looking at the fact that it has been made here in changesets and I've also made it in my work's project:
```
    {
      "files": [".prettierrc", "*.json"],
      "options": {
        "parser": "json",
        "useTabs": false,
        "printWidth": 80
      }
    }
```
I had no idea there was an alternative `"json-stringify"` parser that could be used.

Given though that this setting already conflicts with the package managers I guess we could just treat Changesets doing the same thing as them and thus this shouldn't be a problem. So maybe we should just switch to using `"json-stringify"` parser?
mitchellhamilton: > So maybe we should just switch to using "json-stringify" parser?

I don't see why we would want to use it over just JSON.stringify? From reading what it does, it handles cases where there are commas that shouldn't be there and etc. by using `@babel/parser` which doesn't really matter here?
Andarist: No, there is no added value in that parser (that I can see). I just have meant that, for now, that would be the easiest solution for this problem as we already use prettier here. So in a sense - maybe [the original fix](https://github.com/atlassian/changesets/pull/395/commits/8a5ae4d26d5b07e96f8555a042748ee81b7fc406) by @jonathanmorley should just be restored?
emmenko: It would be nice to fix this formatting issue with the hardcoded value of `printWidth: 20`, as it just messes things up a bit with the default prettier value of `80`.


Andarist: @emmenko could u give more details about the issue you are experiencing?
emmenko: Simply put, our project doesn't specify the `printWidth`, so it uses the default one of `80`. Therefore, the `package.json` files are formatted in a certain way.
Then when Changeset bumps the versions it runs prettier with the `printWidth: 20`, which introduces an unnecessary formatting change.

For example
<img width="1653" alt="image" src="https://user-images.githubusercontent.com/1110551/98216139-f5fc6f80-1f48-11eb-947b-c8a81f1d55c7.png">

Andarist: @emmenko do u have any custom formatting specified for `.json` files? Couldn't you just reformat those however you like using precommit hook with husky + lint-staged or similar tools? The current line of my thinking aligns with @mitchellhamilton 
 - we shouldn't use Prettier here, but rather `JSON.stringify` directly which should match package managers' behavior. Given that this is their behavior and you would usually use them to bump packages such a change would not be a DX downgrade when comparing to using package managers directly. 
jonathanmorley: While updating this PR to use `JSON.stringify` directly (and not use `prettier`), I realised that `JSON.stringify` defaults to not having any whitespace (newlines, indentation, etc).

We would therefore have to make other decisions as to whether we want that whitespace, how much of it we want, and whether we want tabs or spaces.

Any of these could/would run afoul of users specific choices that don't align.

I still think that the current solution assumes the least about the package.jsons in the repos, and tries to make the minimal changes required.
emmenko: Maybe we should just introduce some kind of "hook" configuration so that users can specify their own way of formatting things.
Then changesets simply uses `JSON.stringify` (or whatever reasonable default) and users can opt-in to "patch" the formatting as an "after" step. Conceptually similar to what `lint-staged` does.

Example option in `.changeset/config.json` (the option name is not important):

```json
"formatterCommand": "prettier --write --parser json"
```

It's assumed that the file path is provided to the command.
jonathanmorley: I see this discussion as being how to balance 'internal complexity' and 'external complexity'

The current state of this PR (with the jsonc-parser) increases the internal complexity of the repository, especially compared to just `JSON.stringify` (and even compared to formatting with prettier)

Other suggestions appear to increase the 'external complexity' by requiring new config elements to control the formatting explicitly.

I think we probably need someone like @Andarist to weigh in on which complexity the project would want to focus more on reducing.
mitchellhamilton: We can use detect-indent to get the indentation we should use, this is all that we need to do:

```ts
import * as fs from "fs-extra";
import { Package } from "@manypkg/get-packages";
import path from "path";
import detectIndent from "detect-indent";

async function writePackage(pkg: Package) {
  let pkgRaw = await fs.readFile(path.join(pkg.dir, "package.json"), "utf-8");
  let indent = detectIndent(pkgRaw).indent || "  ";
  return fs.writeFile(
    path.join(pkg.dir, "package.json"),
    JSON.stringify(pkg.packageJson, null, indent) +
      (pkgRaw.endsWith("\n") ? "\n" : "")
  );
}
Andarist: Right, and this is what package managers do. For example:
https://github.com/pnpm/pnpm/blob/cb6e6fc8aa6dd4648e3fee5e54c75ce14d24d9d1/packages/write-project-manifest/src/index.ts#L29-L30

@emmenko I'm not quite sure how a changeset-specific option is really useful here if you can do **the very** same thing by just invoking `prettier` appropriately after invoking `changeset`. This would be pretty much equivalent and doesn't require any documentation or anything whatsoever from the Changesets itself
emmenko: > I'm not quite sure how a changeset-specific option is really useful here if you can do the very same thing by just invoking prettier appropriately after invoking changeset.

The main problem is when using the github action, there is no way to run any custom command **before** the commit is pushed.

If you run `changeset` manually then of course you can just run `prettier` afterwards. 
Andarist: I don't think that's entirely true. You can specify a custom `version` command:
https://github.com/changesets/action/blob/bfeb9e077e6cf393e4c4ef17e2bbc75b308b4364/action.yml#L10-L12
And that should allow you to do what you want.


emmenko: Could you provide an example how that would work?
Andarist: It would look smth like this:

**release.yml**
```yml
name: Release

on:
  push:
    branches:
      - master

jobs:
  release:
    name: Release
    runs-on: ubuntu-latest
    steps:
      - name: Checkout Repo
        uses: actions/checkout@master
        with:
          # This makes Actions fetch all Git history so that Changesets can generate changelogs with the correct commits
          fetch-depth: 0

      - name: Setup Node.js 12.x
        uses: actions/setup-node@master
        with:
          node-version: 12.x

      - name: Install Dependencies
        run: yarn

      - name: Create Release Pull Request or Publish to npm
        id: changesets
        uses: changesets/action@master
        with:
          version: yarn version
          publish: yarn release
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
```

**package.json**
```json
{
  "scripts": {
    "version": "changeset version && prettier '**/package.json' —write"
  }
}
```
emmenko: Thanks I'll give it a try.

Patch:
diff --git a/.changeset/tame-toys-yawn.md b/.changeset/tame-toys-yawn.md
new file mode 100644
index 0000000..808b2c2
--- /dev/null
+++ b/.changeset/tame-toys-yawn.md
@@ -0,0 +1,5 @@
+---
+"@changesets/apply-release-plan": minor
+---
+
+Use `JSON.stringify` to update package.jsons without including modifications from prettier.
diff --git a/__fixtures__/no-trailing-newline/package.json b/__fixtures__/no-trailing-newline/package.json
new file mode 100644
index 0000000..e66dd5c
--- /dev/null
+++ b/__fixtures__/no-trailing-newline/package.json
@@ -0,0 +1,4 @@
+{
+  "name": "pkg-a",
+  "version": "1.0.0"
+}
\ No newline at end of file
diff --git a/__fixtures__/small-array/package.json b/__fixtures__/small-array/package.json
new file mode 100644
index 0000000..ac58841
--- /dev/null
+++ b/__fixtures__/small-array/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "pkg-a",
+  "version": "1.0.0",
+  "files": [
+    "lib"
+  ]
+}
diff --git a/__fixtures__/tab-indent/package.json b/__fixtures__/tab-indent/package.json
new file mode 100644
index 0000000..99d0486
--- /dev/null
+++ b/__fixtures__/tab-indent/package.json
@@ -0,0 +1,4 @@
+{
+	"name": "pkg-a",
+	"version": "1.0.0"
+}
diff --git a/packages/apply-release-plan/package.json b/packages/apply-release-plan/package.json
index fc4c3a9..cae2b04 100644
--- a/packages/apply-release-plan/package.json
+++ b/packages/apply-release-plan/package.json
@@ -13,6 +13,7 @@
     "@changesets/git": "^1.0.5",
     "@changesets/types": "^3.1.0",
     "@manypkg/get-packages": "^1.0.1",
+    "detect-indent": "^6.0.0",
     "fs-extra": "^7.0.1",
     "lodash.startcase": "^4.4.0",
     "outdent": "^0.5.0",
diff --git a/packages/apply-release-plan/src/index.ts b/packages/apply-release-plan/src/index.ts
index 15c81e0..1efc437 100644
--- a/packages/apply-release-plan/src/index.ts
+++ b/packages/apply-release-plan/src/index.ts
@@ -10,6 +10,7 @@ import { defaultConfig } from "@changesets/config";
 import * as git from "@changesets/git";
 import resolveFrom from "resolve-from";
 import { Packages } from "@manypkg/get-packages";
+import detectIndent from "detect-indent";
 
 import fs from "fs-extra";
 import path from "path";
@@ -105,21 +106,13 @@ export default async function applyReleasePlan(
 
   for (let release of finalisedRelease) {
     let { changelog, packageJson, dir, name } = release;
-    let pkgJSONPath = path.resolve(dir, "package.json");
 
-    let changelogPath = path.resolve(dir, "CHANGELOG.md");
-
-    let parsedConfig = prettier.format(JSON.stringify(packageJson), {
-      ...prettierConfig,
-      filepath: pkgJSONPath,
-      parser: "json",
-      printWidth: 20
-    });
-
-    await fs.writeFile(pkgJSONPath, parsedConfig);
+    const pkgJSONPath = path.resolve(dir, "package.json");
+    await updatePackageJson(pkgJSONPath, packageJson);
     touchedFiles.push(pkgJSONPath);
 
     if (changelog && changelog.length > 0) {
+      const changelogPath = path.resolve(dir, "CHANGELOG.md");
       await updateChangelog(changelogPath, changelog, name, prettierConfig);
       touchedFiles.push(changelogPath);
     }
@@ -265,6 +258,18 @@ async function updateChangelog(
   }
 }
 
+async function updatePackageJson(
+  pkgJsonPath: string,
+  pkgJson: any
+): Promise<void> {
+  const pkgRaw = await fs.readFile(pkgJsonPath, "utf-8");
+  const indent = detectIndent(pkgRaw).indent || "  ";
+  const stringified =
+    JSON.stringify(pkgJson, null, indent) + (pkgRaw.endsWith("\n") ? "\n" : "");
+
+  return fs.writeFile(pkgJsonPath, stringified);
+}
+
 async function prependFile(
   filePath: string,
   data: string,


Test patch:
diff --git a/packages/apply-release-plan/src/index.test.ts b/packages/apply-release-plan/src/index.test.ts
index e265076..b31ccd8 100644
--- a/packages/apply-release-plan/src/index.test.ts
+++ b/packages/apply-release-plan/src/index.test.ts
@@ -108,6 +108,65 @@ async function testSetup(
 
 describe("apply release plan", () => {
   describe("versioning", () => {
+    describe("formatting", () => {
+      it("should not reformat a small array in a package.json", async () => {
+        const releasePlan = new FakeReleasePlan();
+        let { changedFiles } = await testSetup(
+          "small-array",
+          releasePlan.getReleasePlan(),
+          releasePlan.config
+        );
+        let pkgPath = changedFiles.find(a => a.endsWith(`package.json`));
+
+        if (!pkgPath) throw new Error(`could not find an updated package json`);
+        let pkgJSON = await fs.readFile(pkgPath, { encoding: "utf-8" });
+
+        expect(pkgJSON).toStrictEqual(`{
+  "name": "pkg-a",
+  "version": "1.1.0",
+  "files": [
+    "lib"
+  ]
+}
+`);
+      });
+      it("should not change tab indentation in a package.json", async () => {
+        const releasePlan = new FakeReleasePlan();
+        let { changedFiles } = await testSetup(
+          "tab-indent",
+          releasePlan.getReleasePlan(),
+          releasePlan.config
+        );
+        let pkgPath = changedFiles.find(a => a.endsWith(`package.json`));
+
+        if (!pkgPath) throw new Error(`could not find an updated package json`);
+        let pkgJSON = await fs.readFile(pkgPath, { encoding: "utf-8" });
+
+        expect(pkgJSON).toStrictEqual(`{
+\t"name": "pkg-a",
+\t"version": "1.1.0"
+}
+`);
+      });
+      it("should not add trailing newlines in a package.json if they don't exist", async () => {
+        const releasePlan = new FakeReleasePlan();
+        let { changedFiles } = await testSetup(
+          "no-trailing-newline",
+          releasePlan.getReleasePlan(),
+          releasePlan.config
+        );
+        let pkgPath = changedFiles.find(a => a.endsWith(`package.json`));
+
+        if (!pkgPath) throw new Error(`could not find an updated package json`);
+        let pkgJSON = await fs.readFile(pkgPath, { encoding: "utf-8" });
+
+        expect(pkgJSON).toStrictEqual(`{
+  "name": "pkg-a",
+  "version": "1.1.0"
+}`);
+      });
+    });
+
     it("should update a version for one package", async () => {
       const releasePlan = new FakeReleasePlan();
       let { changedFiles } = await testSetup(