-
-
Notifications
You must be signed in to change notification settings - Fork 162
fix: skip package.json writting when content is deeply equal #913
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { readFileSync } from 'node:fs' | ||
| import path from 'node:path' | ||
| import { describe, expect, test } from 'vitest' | ||
| import { writeFixtures } from '../../tests/utils.ts' | ||
| import { detectIndentation, writeJsonFile } from './json.ts' | ||
|
|
||
| describe('writeJsonFile', () => { | ||
| test('creates a new file when it does not exist', async (context) => { | ||
| const { testDir } = await writeFixtures(context, { 'placeholder.txt': '' }) | ||
| const filePath = path.join(testDir, 'new.json') | ||
| writeJsonFile(filePath, { foo: 'bar' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe('{\n "foo": "bar"\n}') | ||
| }) | ||
|
|
||
| test('does not rewrite when keys are reordered but content is deeply equal', async (context) => { | ||
| const original = '{"b":1,\n"a":2}' | ||
| const { testDir } = await writeFixtures(context, { 'pkg.json': original }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { a: 2, b: 1 }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe(original) | ||
| }) | ||
|
|
||
| test('does not rewrite when content is identical', async (context) => { | ||
| const original = '{\t"foo":"bar"\n }' | ||
| const { testDir } = await writeFixtures(context, { 'pkg.json': original }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { foo: 'bar' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe(original) | ||
| }) | ||
|
|
||
| test('updates the file when content changes', async (context) => { | ||
| const { testDir } = await writeFixtures(context, { | ||
| 'pkg.json': '{\n "foo": "bar"\n}', | ||
| }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { foo: 'baz' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe('{\n "foo": "baz"\n}') | ||
| }) | ||
|
|
||
| test('preserves tab indentation', async (context) => { | ||
| const { testDir } = await writeFixtures(context, { | ||
| 'pkg.json': '{\n\t"foo": "bar"\n}', | ||
| }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { foo: 'baz' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe('{\n\t"foo": "baz"\n}') | ||
| }) | ||
|
|
||
| test('preserves 4-space indentation', async (context) => { | ||
| const { testDir } = await writeFixtures(context, { | ||
| 'pkg.json': '{\n "foo": "bar"\n}', | ||
| }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { foo: 'baz' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe('{\n "foo": "baz"\n}') | ||
| }) | ||
|
|
||
| test('preserves CRLF line endings', async (context) => { | ||
| const { testDir } = await writeFixtures(context, { | ||
| 'pkg.json': '{\r\n "foo": "bar"\r\n}', | ||
| }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { foo: 'baz' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe('{\r\n "foo": "baz"\r\n}') | ||
| }) | ||
|
|
||
| test('preserves trailing newline', async (context) => { | ||
| const { testDir } = await writeFixtures(context, { | ||
| 'pkg.json': '{\n "foo": "bar"\n}\n', | ||
| }) | ||
| const filePath = path.join(testDir, 'pkg.json') | ||
| writeJsonFile(filePath, { foo: 'baz' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe('{\n "foo": "baz"\n}\n') | ||
| }) | ||
| }) | ||
|
|
||
| describe('detectIndent', () => { | ||
| test('two spaces', ({ expect }) => { | ||
| expect(detectIndentation(stringifyJson(2))).toBe(2) | ||
| }) | ||
| test('four spaces', ({ expect }) => { | ||
| expect(detectIndentation(stringifyJson(4))).toBe(4) | ||
| }) | ||
| test('tab', ({ expect }) => { | ||
| expect(detectIndentation(stringifyJson('\t'))).toBe('\t') | ||
| }) | ||
| test('empty', ({ expect }) => { | ||
| expect(detectIndentation('')).toBe(2) | ||
| }) | ||
| test('empty line', ({ expect }) => { | ||
| expect(detectIndentation('{\n\n "foo": 42 }')).toBe(2) | ||
| }) | ||
| }) | ||
|
|
||
| function stringifyJson(indentation: string | number): string { | ||
| const contents = JSON.stringify({ foo: 42 }, null, indentation) | ||
| return contents | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||||||||||||||||||
| import { readFileSync, writeFileSync } from 'node:fs' | ||||||||||||||||||||||
| import { isDeepStrictEqual } from 'node:util' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function writeJsonFile(filePath: string, content: unknown): void { | ||||||||||||||||||||||
| let originalContent: unknown = undefined | ||||||||||||||||||||||
| let originalIndent: string | number = 2 | ||||||||||||||||||||||
| let originalEOL: string = '\n' | ||||||||||||||||||||||
| let originalHasTrailingNewline: boolean = false | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| const text = readFileSync(filePath, 'utf8') | ||||||||||||||||||||||
| originalContent = JSON.parse(text) | ||||||||||||||||||||||
| originalIndent = detectIndentation(text) | ||||||||||||||||||||||
| if (text.includes('\r\n')) { | ||||||||||||||||||||||
| originalEOL = '\r\n' | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if (text.endsWith('\n')) { | ||||||||||||||||||||||
| originalHasTrailingNewline = true | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||
| // File doesn't exist or isn't valid JSON, we'll overwrite it with our content | ||||||||||||||||||||||
|
Comment on lines
+20
to
+21
|
||||||||||||||||||||||
| } catch { | |
| // File doesn't exist or isn't valid JSON, we'll overwrite it with our content | |
| } catch (error) { | |
| if (error instanceof SyntaxError) { | |
| // The file isn't valid JSON, we'll overwrite it with our content | |
| } else if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | |
| // The file doesn't exist, we'll overwrite it with our content | |
| } else { | |
| throw error | |
| } |
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deep-equality short-circuit is guarded by a truthy check (originalContent && ...), which means valid JSON roots like null, false, 0, or "" will never be treated as equal and will be rewritten unnecessarily. Also, callers may include undefined properties (e.g. objects that rely on JSON.stringify omitting them), which will make isDeepStrictEqual fail even when the on-disk JSON and the serialized output are identical. Consider tracking whether parsing succeeded separately (instead of truthiness) and normalizing content to JSON-serializable form (e.g. by removing undefined keys) before comparing.
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.stringify can return undefined for inputs like undefined, functions, or symbols. In that case writeFileSync will throw with a less helpful error. Since this helper is exported and accepts unknown, consider narrowing the parameter type to JSON-serializable values and/or validating the result of JSON.stringify to throw a clearer error before writing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a test covering the case where
contentcontains properties with valueundefined(whichJSON.stringifyomits). With the current deep-equality check, this can cause a rewrite even if the serialized JSON would be identical to the existing file (relevant for callers that “remove” fields viafoo: undefined). Adding a fixture/test for this scenario would prevent regressions.