feat(core): preserve formatting when applying fixes

Closes #241 Closes #195
JamieMason · Aug 25, 2024 · 8322d1c · 8322d1c
1 parent aac4e24
commit 8322d1c
Show file tree

Hide file tree

Showing 17 changed files with 106 additions and 67 deletions.
diff --git a/.zed/tasks.json b/.zed/tasks.json
@@ -1,13 +1,4 @@
 [
-  {
-    "label": "Add test for current file",
-    "command": "$ZED_WORKTREE_ROOT/node_modules/.bin/tsx",
-    "args": ["$ZED_WORKTREE_ROOT/scripts/add-test.ts", "$ZED_RELATIVE_FILE"],
-    "shell": "system",
-    "reveal": "never",
-    "use_new_terminal": true,
-    "hide": "always"
-  },
   {
     "label": "Vitest run",
     "command": "$ZED_WORKTREE_ROOT/node_modules/.bin/vitest",

diff --git a/package.json b/package.json
@@ -53,6 +53,7 @@
     "enquirer": "2.4.1",
     "fast-check": "3.21.0",
     "globby": "14.0.2",
+    "jsonc-parser": "3.3.1",
     "minimatch": "10.0.1",
     "npm-package-arg": "11.0.3",
     "ora": "8.0.1",

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/src/bin-format/format.ts b/src/bin-format/format.ts
@@ -15,6 +15,7 @@ import type { PackageJson } from '../get-package-json-files/package-json-file.js
 import { exitIfInvalid } from '../io/exit-if-invalid.js';
 import type { Io } from '../io/index.js';
 import { IoTag } from '../io/index.js';
+import { toFormattedJson } from '../io/to-formatted-json.js';
 import { writeIfChanged } from '../io/write-if-changed.js';
 import { withLogger } from '../lib/with-logger.js';
 
@@ -106,6 +107,8 @@ export function pipeline(ctx: Ctx): Effect.Effect<Ctx> {
       const sortedKeys = new Set([...sortFirst, ...otherKeys]);
       sortObject(sortedKeys, contents);
     }
+
+    file.nextJson = toFormattedJson(ctx, file);
   });
 
   return Effect.succeed(ctx);

diff --git a/src/bin-lint/lint.ts b/src/bin-lint/lint.ts
@@ -12,7 +12,7 @@ import { getContext } from '../get-context/index.js';
 import { exitIfInvalid } from '../io/exit-if-invalid.js';
 import type { Io } from '../io/index.js';
 import { IoTag } from '../io/index.js';
-import { toJson } from '../io/to-json.js';
+import { toFormattedJson } from '../io/to-formatted-json.js';
 import { withLogger } from '../lib/with-logger.js';
 
 interface Input {
@@ -24,6 +24,29 @@ interface Input {
 export function lint({ io, cli, errorHandlers = defaultErrorHandlers }: Input) {
   return pipe(
     getContext({ io, cli, errorHandlers }),
+    // Formatting
+    Effect.flatMap(ctx =>
+      Effect.gen(function* ($) {
+        if (ctx.config.rcFile.lintFormatting !== false) {
+          yield* $(Effect.logInfo(chalk`{yellow Formatting}`));
+          yield* $(format(ctx));
+          for (const file of ctx.packageJsonFiles) {
+            const shortPath = file.jsonFile.shortPath;
+            const formattedJson = toFormattedJson(ctx, file);
+            const isFormatted = file.jsonFile.json === formattedJson;
+            if (isFormatted) {
+              yield* $(
+                Effect.logInfo(chalk`{green ${ICON.tick}} ${shortPath}`),
+              );
+            } else {
+              ctx.isInvalid = true;
+              yield* $(Effect.logInfo(chalk`{red ${ICON.cross}} ${shortPath}`));
+            }
+          }
+        }
+        return ctx;
+      }),
+    ),
     // Versions
     Effect.flatMap(ctx =>
       Effect.gen(function* ($) {
@@ -44,29 +67,6 @@ export function lint({ io, cli, errorHandlers = defaultErrorHandlers }: Input) {
         return ctx;
       }),
     ),
-    // Formatting
-    Effect.flatMap(ctx =>
-      Effect.gen(function* ($) {
-        if (ctx.config.rcFile.lintFormatting !== false) {
-          yield* $(Effect.logInfo(chalk`{yellow Formatting}`));
-          yield* $(format(ctx));
-          for (const file of ctx.packageJsonFiles) {
-            const nextJson = toJson(ctx, file);
-            const hasChanged = file.jsonFile.json !== nextJson;
-            const shortPath = file.jsonFile.shortPath;
-            if (hasChanged) {
-              ctx.isInvalid = true;
-              yield* $(Effect.logInfo(chalk`{red ${ICON.cross}} ${shortPath}`));
-            } else {
-              yield* $(
-                Effect.logInfo(chalk`{green ${ICON.tick}} ${shortPath}`),
-              );
-            }
-          }
-        }
-        return ctx;
-      }),
-    ),
     Effect.flatMap(exitIfInvalid),
     Effect.provide(
       pipe(

diff --git a/src/error-handlers/default-error-handlers.spec.ts b/src/error-handlers/default-error-handlers.spec.ts
@@ -108,7 +108,13 @@ describe('Broken Projects', () => {
         await Effect.runPromiseExit(fixMismatches(scenario));
         expect(scenario.errorHandlers.JsonParseError).toHaveBeenCalledWith({
           _tag: 'JsonParseError',
-          error: expect.any(SyntaxError),
+          errors: expect.arrayContaining([
+            {
+              error: expect.any(Number),
+              length: expect.any(Number),
+              offset: expect.any(Number),
+            },
+          ]),
           filePath: expect.stringContaining('/package.json'),
           json: '}INVALID{',
         });

diff --git a/src/error-handlers/default-error-handlers.ts b/src/error-handlers/default-error-handlers.ts
@@ -47,7 +47,7 @@ export const defaultErrorHandlers: ErrorHandlers = {
       [
         chalk.red('An error was found when parsing a JSON file'),
         chalk.red('  File:', err.filePath),
-        chalk.red('  Error:', err.error),
+        chalk.red('  Error:', err.errors),
       ].join(EOL),
     );
   },

diff --git a/src/get-context/get-context.spec.ts b/src/get-context/get-context.spec.ts
@@ -27,9 +27,16 @@ it('errors when package.json is invalid', async () => {
     },
   })();
   await Effect.runPromiseExit(pipe(getContext(scenario), Effect.merge));
+
   expect(scenario.errorHandlers.JsonParseError).toHaveBeenCalledWith(
     new JsonParseError({
-      error: expect.any(SyntaxError),
+      errors: expect.arrayContaining([
+        {
+          error: expect.any(Number),
+          length: expect.any(Number),
+          offset: expect.any(Number),
+        },
+      ]),
       filePath: expect.stringContaining('/package.json') as unknown as string,
       json: 'THIS-IS-NOT-VALID-JSON',
     }),

diff --git a/src/get-package-json-files/package-json-file.ts b/src/get-package-json-files/package-json-file.ts
@@ -1,4 +1,5 @@
 import { Effect, pipe } from 'effect';
+import { applyEdits, modify } from 'jsonc-parser';
 import type { Strategy } from '../config/get-custom-types.js';
 import type { Ctx } from '../get-context/index.js';
 import { Instance } from '../get-instances/instance.js';
@@ -43,11 +44,15 @@ export class PackageJsonFile {
   /** the .name property from the package.json file */
   name: string | undefined;
 
+  /** the next package.json file contents after modification, with formatting preserved */
+  nextJson: string;
+
   constructor(jsonFile: JsonFile<PackageJson>, config: Ctx['config']) {
     this._instances = null;
     this.config = config;
     this.jsonFile = jsonFile;
     this.name = jsonFile.contents.name;
+    this.nextJson = jsonFile.json;
   }
 
   getInstances(enabledTypes: Strategy.Any[]): Effect.Effect<Instance[]> {
@@ -87,4 +92,22 @@ export class PackageJsonFile {
     }
     return Effect.succeed(this._instances);
   }
+
+  /**
+   * Apply an edit to the raw JSON string which will be written to disk. This string preserves the
+   * original formatting of the file.
+   */
+  applyEdit(fullPath: string[], value: string | undefined): void {
+    const edits = modify(
+      this.nextJson,
+      fullPath.map(parseNumericStrings),
+      value,
+      {},
+    );
+    this.nextJson = applyEdits(this.nextJson, edits);
+  }
+}
+
+function parseNumericStrings(key: string): string | number {
+  return /[^0-9]/.test(key) ? key : Number(key);
 }
diff --git a/src/io/read-json-file-sync.ts b/src/io/read-json-file-sync.ts
@@ -1,11 +1,12 @@
 import { dirname, relative } from 'node:path';
 import { Data, Effect, pipe } from 'effect';
+import { type ParseError, parse } from 'jsonc-parser';
 import type { Io } from './index.js';
 import type { ReadFileError } from './read-file-sync.js';
 import { readFileSync } from './read-file-sync.js';
 
 export class JsonParseError extends Data.TaggedClass('JsonParseError')<{
-  readonly error: unknown;
+  readonly errors: ParseError[];
   readonly filePath: string;
   readonly json: string;
 }> {}
@@ -30,12 +31,13 @@ export function readJsonFileSync<T>(
   return pipe(
     Effect.Do,
     Effect.bind('json', () => readFileSync(io, filePath)),
-    Effect.bind('contents', ({ json }) =>
-      Effect.try({
-        try: () => JSON.parse(json),
-        catch: error => new JsonParseError({ error, filePath, json }),
-      }),
-    ),
+    Effect.bind('contents', ({ json }) => {
+      const errors: ParseError[] = [];
+      const data = parse(json, errors);
+      return errors.length === 0
+        ? Effect.succeed(data)
+        : Effect.fail(new JsonParseError({ errors, filePath, json }));
+    }),
     Effect.map(
       ({ contents, json }) =>
         new JsonFile<T>({

diff --git a/src/io/to-json.ts → src/io/to-formatted-json.ts b/src/io/to-json.ts → src/io/to-formatted-json.ts
@@ -30,7 +30,7 @@ export const newlines = {
   },
 };
 
-export function toJson(ctx: Ctx, file: PackageJsonFile): string {
+export function toFormattedJson(ctx: Ctx, file: PackageJsonFile): string {
   const contents = file.jsonFile.contents;
   const indent = getIndent(ctx.config);
   const eol = newlines.detect(file.jsonFile.json);

diff --git a/src/io/write-if-changed.ts b/src/io/write-if-changed.ts
@@ -4,7 +4,6 @@ import { ICON } from '../constants.js';
 import type { Ctx } from '../get-context/index.js';
 import type { PackageJsonFile } from '../get-package-json-files/package-json-file.js';
 import type { Io } from './index.js';
-import { toJson } from './to-json.js';
 import type { WriteFileError } from './write-file-sync.js';
 import { writeFileSync } from './write-file-sync.js';
 
@@ -14,27 +13,18 @@ export function writeIfChanged(
   return pipe(
     Effect.all(
       ctx.packageJsonFiles.map((file: PackageJsonFile) =>
-        pipe(
-          Effect.Do,
-          Effect.bind('nextJson', () => Effect.succeed(toJson(ctx, file))),
-          Effect.bind('hasChanged', ({ nextJson }) =>
-            Effect.succeed(file.jsonFile.json !== nextJson),
-          ),
-          Effect.flatMap(({ hasChanged, nextJson }) =>
-            hasChanged
-              ? pipe(
-                  writeFileSync(file.jsonFile.filePath, nextJson),
-                  Effect.flatMap(() =>
-                    Effect.logInfo(
-                      chalk`{green ${ICON.tick}} ${file.jsonFile.shortPath}`,
-                    ),
-                  ),
-                )
-              : Effect.logInfo(
-                  chalk`{dim ${ICON.skip} ${file.jsonFile.shortPath}}`,
+        file.jsonFile.json !== file.nextJson
+          ? pipe(
+              writeFileSync(file.jsonFile.filePath, file.nextJson),
+              Effect.flatMap(() =>
+                Effect.logInfo(
+                  chalk`{green ${ICON.tick}} ${file.jsonFile.shortPath}`,
                 ),
-          ),
-        ),
+              ),
+            )
+          : Effect.logInfo(
+              chalk`{dim ${ICON.skip} ${file.jsonFile.shortPath}}`,
+            ),
       ),
     ),
     Effect.map(() => ctx),

diff --git a/src/strategy/name-and-version-props.ts b/src/strategy/name-and-version-props.ts
@@ -77,6 +77,7 @@ export class NameAndVersionPropsStrategy {
         Effect.flatMap(parent =>
           Effect.try(() => {
             parent[key] = nextValue;
+            file.applyEdit(fullPath, nextValue);
           }),
         ),
         Effect.tapError(() =>
@@ -93,6 +94,7 @@ export class NameAndVersionPropsStrategy {
       Effect.flatMap(parent =>
         Effect.try(() => {
           parent[this.path] = nextValue;
+          file.applyEdit([this.path], nextValue);
         }),
       ),
       Effect.tapError(() =>

diff --git a/src/strategy/named-version-string.ts b/src/strategy/named-version-string.ts
@@ -63,6 +63,7 @@ export class NamedVersionStringStrategy {
         Effect.flatMap(parent =>
           Effect.try(() => {
             parent[key] = nextValue;
+            file.applyEdit(fullPath, nextValue);
           }),
         ),
         Effect.tapError(() =>
@@ -79,6 +80,7 @@ export class NamedVersionStringStrategy {
       Effect.flatMap(parent =>
         Effect.try(() => {
           parent[this.path] = nextValue;
+          file.applyEdit([this.path], nextValue);
         }),
       ),
       Effect.tapError(() =>

diff --git a/src/strategy/unnamed-version-string.ts b/src/strategy/unnamed-version-string.ts
@@ -54,6 +54,7 @@ export class UnnamedVersionStringStrategy {
         Effect.flatMap(parent =>
           Effect.try(() => {
             parent[key] = nextValue;
+            file.applyEdit(fullPath, nextValue);
           }),
         ),
         Effect.tapError(() =>
@@ -70,6 +71,7 @@ export class UnnamedVersionStringStrategy {
       Effect.flatMap(parent =>
         Effect.try(() => {
           parent[this.path] = nextValue;
+          file.applyEdit([this.path], nextValue);
         }),
       ),
       Effect.tapError(() =>

diff --git a/src/strategy/versions-by-name.ts b/src/strategy/versions-by-name.ts
@@ -37,12 +37,14 @@ export class VersionsByNameStrategy {
     [name, version]: [string, string | Delete],
   ): Effect.Effect<PackageJsonFile> {
     const nextValue = version === DELETE ? undefined : version;
+    const fullPath = this.path.split('.');
     return pipe(
-      get(file.jsonFile.contents, ...this.path.split('.')),
+      get(file.jsonFile.contents, ...fullPath),
       Effect.flatMap(getOptionOfNonEmptyObject),
       Effect.flatMap(parent =>
         Effect.try(() => {
           parent[name] = nextValue;
+          file.applyEdit(fullPath.concat(name), nextValue);
         }),
       ),
       Effect.tapError(() =>

diff --git a/test/lib/create-scenario.ts b/test/lib/create-scenario.ts
@@ -16,7 +16,7 @@ import type {
   PackageJsonFile,
 } from '../../src/get-package-json-files/package-json-file.js';
 import type { Io } from '../../src/io/index.js';
-import { newlines } from '../../src/io/to-json.js';
+import { newlines } from '../../src/io/to-formatted-json.js';
 import type { Report } from '../../src/report.js';
 
 type NodeFs = typeof fs;