fix: skip package.json writting when content is deeply equal#913
Conversation
✅ Deploy Preview for tsdown-main ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
tsdown
create-tsdown
@tsdown/css
@tsdown/exe
tsdown-migrate
commit: |
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent unnecessary package.json rewrites (notably when exports.devExports is enabled) by switching the “outdated” check from string comparison to deep structural equality, while preserving existing formatting details (indentation, EOLs, trailing newline) when writing.
Changes:
- Added a
writeJsonFilehelper that reads existing JSON, preserves formatting, and skips writes when content is deeply equal. - Updated
writeExportsto usewriteJsonFileinstead of manual stringify/compare/write logic. - Added Vitest coverage for the new JSON writing helper (creation, no-rewrite, formatting preservation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/utils/json.ts | Introduces JSON write helper with formatting preservation and deep-equality skip. |
| src/utils/json.test.ts | Adds tests validating no-rewrite behavior and formatting preservation. |
| src/features/pkg/exports.ts | Replaces manual package.json rewrite logic with writeJsonFile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (originalContent && isDeepStrictEqual(originalContent, content)) { | ||
| // The content is the same. We just return without updating the file format | ||
| return | ||
| } |
There was a problem hiding this comment.
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.
| } catch { | ||
| // File doesn't exist or isn't valid JSON, we'll overwrite it with our content |
There was a problem hiding this comment.
The bare catch {} swallows all errors from readFileSync/JSON.parse, including permission errors, transient IO failures, and other unexpected conditions. That can mask real operational problems and then proceed to overwrite the file with default formatting. It would be safer to only ignore the specific cases you expect (e.g. missing file / invalid JSON) and rethrow anything else.
| } 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 | |
| } |
| let jsonString = JSON.stringify(content, null, originalIndent) | ||
| if (originalEOL !== '\n') { | ||
| jsonString = jsonString.replaceAll('\n', originalEOL) | ||
| } | ||
| if (originalHasTrailingNewline) { | ||
| jsonString += originalEOL | ||
| } | ||
|
|
||
| writeFileSync(filePath, jsonString, 'utf8') |
There was a problem hiding this comment.
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.
| writeJsonFile(filePath, { foo: 'bar' }) | ||
| expect(readFileSync(filePath, 'utf8')).toBe(original) | ||
| }) | ||
|
|
There was a problem hiding this comment.
There isn't a test covering the case where content contains properties with value undefined (which JSON.stringify omits). 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 via foo: undefined). Adding a fixture/test for this scenario would prevent regressions.
| test('does not rewrite when content only adds undefined properties', async (context) => { | |
| const original = '{\n "foo": "bar"\n}' | |
| const { testDir } = await writeFixtures(context, { 'pkg.json': original }) | |
| const filePath = path.join(testDir, 'pkg.json') | |
| writeJsonFile(filePath, { foo: 'bar', removed: undefined }) | |
| expect(readFileSync(filePath, 'utf8')).toBe(original) | |
| }) |
Description
When
exports.devExportsis enabled,tsdownwill try to updatepackage.jsonif it's outdated.Previously, the outdated status is using string comparing here.
In this PR, the outdated check is using
isDeepStrictEqualfromnode:utils(added in Node.js v9).These changes ensure that
tsdownwon't rewritepackage.jsonif thepackage.jsonsomehow has weird formatting, such as specifically ordering inpublishConfig.exports, or mixing the use of spaces and\tas indentation.Linked Issues
Additional context