Upgrade: Preserve package.json indentation when upgrading#32280
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
valentinpalkovic
changed the title
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
@y-hsgw when testing a locally built CLI on your branch, I am still seeing changes to package.json indentation. Could you please update testing instructions? @ndelangen I'm gonna be OOO tonight and originally wanted to take this PR's review because I had feedback during the weekly triage. Is it ok if I give it to you so we don't make Yukihiro wait too long? 🙏 |
|
@Sidnioulz |
|
View your CI Pipeline Execution ↗ for commit 89a7f09 ☁️ Nx Cloud last updated this comment at |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Testing using the local CLI from a sandbox:
cd code
yarn task compile
cd ../sandbox/cra-default-js
node ../../code/lib/cli-storybook/dist/bin/index.js upgradeIt seems to work for tabs and for e.g. 8 spaces, but not for 0 spaces. This is likely a limitation of the detect-indent package.
|
@ndelangen I've successfullyr reviwed and locally tested the PR. Leaving it in your hands as I don't have push permission! |
…pgrade-package-json-indent
📝 WalkthroughWalkthroughEnhances JsPackageManager to detect and preserve indentation when reading and writing package.json files using the detect-indent library. Indentation metadata is stored non-enumerably on cached package.json objects using a Symbol key and applied dynamically during serialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
169-176: Use a namespaced global symbol and guard against spreadingundefineddependency objects
- Namespacing:
Symbol.for('indent')is very generic and global. Prefer a namespaced key to avoid accidental collisions.- Safety:
{ ...packageJSON.dependencies }will throw if the field isundefined.Apply this diff:
- packageJSON[Symbol.for('indent')] = detectIndent(jsonContent).indent ?? 2; + const INDENT_SYM = Symbol.for('storybook:package-json:indent'); + packageJSON[INDENT_SYM] = detectIndent(jsonContent).indent ?? 2; @@ - return { - ...packageJSON, - dependencies: { ...packageJSON.dependencies }, - devDependencies: { ...packageJSON.devDependencies }, - peerDependencies: { ...packageJSON.peerDependencies }, - }; + return { + ...packageJSON, + dependencies: { ...(packageJSON.dependencies ?? {}) }, + devDependencies: { ...(packageJSON.devDependencies ?? {}) }, + peerDependencies: { ...(packageJSON.peerDependencies ?? {}) }, + };
🧹 Nitpick comments (2)
code/core/src/common/js-package-manager/JsPackageManager.ts (2)
179-186: Avoid unnecessary JSON parse: detect indent directly from file content
#getIndentcan compute indentation from the file content without parsing JSON (cheaper and more robust when JSON is temporarily invalid).- #getIndent(filePath: string): string | number { - try { - const packageJson = JsPackageManager.getPackageJson(filePath); - return packageJson[Symbol.for('indent')]; - } catch (e) { - return 2; - } - } + #getIndent(filePath: string): string | number { + try { + const src = readFileSync(filePath, 'utf8'); + return detectIndent(src).indent ?? 2; + } catch { + return 2; + } + }
198-201: Minor: reusefilePathfor write; add tests to lock behavior
- Nit: you already compute
filePath; use it forwriteFileSyncfor consistency.- Please add tests covering tabs, 2/4-space, minified (no indent/empty string), missing-destination file (fallback to 2).
- const filePath = join(directory, 'package.json'); + const filePath = join(directory, 'package.json'); const indent = this.#getIndent(filePath); const content = `${JSON.stringify(packageJsonToWrite, null, indent)}\n`; - writeFileSync(resolve(directory, 'package.json'), content, 'utf8'); + writeFileSync(filePath, content, 'utf8');Happy to draft a Jest test suite for these cases if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/core/src/common/js-package-manager/JsPackageManager.ts(4 hunks)code/core/src/csf-tools/CsfFile.test.ts(0 hunks)code/core/src/outline/outlineCSS.ts(1 hunks)code/core/src/types/modules/core-common.ts(1 hunks)code/frameworks/angular/build-schema.json(5 hunks)code/frameworks/angular/start-schema.json(6 hunks)code/frameworks/sveltekit/tsconfig.json(1 hunks)code/renderers/vue3/src/extractArgTypes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- code/core/src/csf-tools/CsfFile.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/types/modules/core-common.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/renderers/vue3/src/extractArgTypes.tscode/core/src/outline/outlineCSS.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/types/modules/core-common.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/renderers/vue3/src/extractArgTypes.tscode/core/src/outline/outlineCSS.ts
code/**/tsconfig*.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep TypeScript strict mode enabled; do not relax strict compiler options in tsconfig files
Files:
code/frameworks/sveltekit/tsconfig.json
🧬 Code graph analysis (1)
code/core/src/outline/outlineCSS.ts (1)
scripts/utils/tools.ts (1)
dedent(118-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/renderers/vue3/src/extractArgTypes.ts (1)
352-353: Doc example tweak looks goodUpdated example formatting keeps the intent clear and consistent with surrounding docs.
code/core/src/outline/outlineCSS.ts (1)
7-7: Whitespace trim is harmlessSpacing cleanup around the
dedenttag retains the same runtime behavior.code/frameworks/sveltekit/tsconfig.json (1)
5-7: Valid JSON restoredRemoving the trailing comma fixes the invalid JSON—thanks for tightening this up.
code/frameworks/angular/build-schema.json (1)
70-197: Schema formatting change looks goodCollapsing the array literals keeps the schema equivalent while matching the new formatting.
code/frameworks/angular/start-schema.json (1)
97-233: Consistent formatting maintainedThe schema keeps its behavior; the tightened array formatting mirrors the build schema updates.
| static getPackageJson(packageJsonPath: string): PackageJsonWithDepsAndDevDeps { | ||
| const jsonContent = readFileSync(packageJsonPath, 'utf8'); | ||
| const packageJSON = JSON.parse(jsonContent); | ||
| packageJSON[Symbol.for('indent')] = detectIndent(jsonContent).indent ?? 2; |
There was a problem hiding this comment.
This feels rather hacky to me. I get that it works, but it's very unexpected behavior.
There was a problem hiding this comment.
It's probably gonna be less often wrong than it was before, and we use the same hack in the ESLint plugin so we're not increasing our maintenance surface wrt. dependencies.
I personally think it's nice to reformat a file after doing codegen on it, and this is likely as close as we can get without knowing what prettifier end users are using.
There was a problem hiding this comment.
I'm okay with this trick as long as it's clarified why this uses a Symbol. A code comment along the lines of "Using a Symbol key so JSON.stringify will omit it later" would suffice. It wasn't obvious to me that this is how stringify behaves, I had to manually verify.
|
Thank you for your contribution! Could you please resolve the merge conflicts? |
…pgrade-package-json-indent
|
@valentinpalkovic |
|
@y-hsgw thanks! I have re-run the failing tests. I also believe they are unrelated. If re-running them several times does not allow them to pass, then it will mean that it is caused by your code (though I fail to see how). |
| static getPackageJson(packageJsonPath: string): PackageJsonWithDepsAndDevDeps { | ||
| const jsonContent = readFileSync(packageJsonPath, 'utf8'); | ||
| const packageJSON = JSON.parse(jsonContent); | ||
| packageJSON[Symbol.for('indent')] = detectIndent(jsonContent).indent ?? 2; |
There was a problem hiding this comment.
I'm okay with this trick as long as it's clarified why this uses a Symbol. A code comment along the lines of "Using a Symbol key so JSON.stringify will omit it later" would suffice. It wasn't obvious to me that this is how stringify behaves, I had to manually verify.
| #getIndent(filePath: string): string | number { | ||
| try { | ||
| const packageJson = JsPackageManager.getPackageJson(filePath); | ||
| return packageJson[Symbol.for('indent')]; |
There was a problem hiding this comment.
There is no need to use Symbol.for. Just define it as a constant at the top of the file and reference that.
| return packageJson[Symbol.for('indent')]; | |
| return packageJson[indentSymbol]; |
With at the top of the file:
const indentSymbol = Symbol('indent');| } | ||
|
|
||
| export type PackageJson = PackageJsonFromTypeFest & Record<string, any>; | ||
| export type PackageJson = PackageJsonFromTypeFest & Record<string | symbol, any>; |
There was a problem hiding this comment.
I don't think we should be changing the type for something that's purely internal. Just extend PackageJsonWithDepsAndDevDeps where needed.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
ndelangen
dismissed
ghengeveld’s stale review
Your review comment was:
I'm okay with this trick as long as it's clarified why this uses a Symbol. A code comment along the lines of "Using a Symbol key so JSON.stringify will omit it later" would suffice. It wasn't obvious to me that this is how stringify behaves, I had to manually verify.
As far as I can see, these 2 issues were fully resolved, so on that account I'm dismissing your review.
Since you're OOO for the next few days.
5 participants
Closes #25812
What I did
This PR fixes an issue where running storybook upgrade reformatted package.json by changing tab indentation to 2 spaces.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
react-vite-default-ts/package.jsonto tabs{ "scripts": { // ...existing scripts "upgrade-storybook": "storybook upgrade" } }Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR addresses a user experience issue where the Storybook upgrade process was reformatting package.json files by converting tab indentation to 2-space indentation. The fix modifies the
writePackageJsonmethod in theJsPackageManagerclass to preserve the original indentation style of package.json files.The change introduces the
detect-indentpackage to analyze the existing package.json file's indentation pattern before writing the updated version. Instead of always using a hardcoded 2-space indentation (JSON.stringify(packageJsonToWrite, null, 2)), the code now:detect-indentto determine the current indentation style (tabs, 2 spaces, 4 spaces, etc.)This fix is part of the core package manager functionality that handles package.json modifications during Storybook upgrades. The
JsPackageManagerclass is a foundational component used across different package managers (npm, yarn, pnpm) to handle package.json operations, making this fix universally applicable to all Storybook upgrade scenarios.Confidence score: 4/5
detect-indent) for a specific, well-defined problemJsPackageManager.tsfile requires attention to ensure thedetect-indentdependency is properly added to the package.jsonSummary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.