Build: Move prettier to oxfmt#34183
Conversation
|
View your CI Pipeline Execution ↗ for commit f9df5e7
☁️ Nx Cloud last updated this comment at |
This reverts commit 2c40162.
|
Had to revert codemods because pipeline was failing ? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Prettier with oxfmt across the repo: adds .oxfmtrc.json files and oxfmt scripts, removes Prettier configs/dependencies/exports, updates editor/CI to use oxfmt, and updates scripts, generators, and many tests/snapshots to call or consume oxfmt-formatted output. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Repo as Repository
participant CI as GitHub Actions
participant Scripts as Repo scripts (generators, linters)
participant Oxfmt as oxfmt
Dev->>Repo: push changes (oxfmt config, scripts, code)
CI->>Repo: checkout
CI->>Scripts: run `yarn lint:fmt`
Scripts->>Oxfmt: format (filename/content, options)
Oxfmt-->>Scripts: return { code: formattedContent }
Scripts->>Repo: write formatted outputs / continue pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
AGENTS.md (1)
237-237: Document file-specific formatting syntax for oxfmt.The new instruction
cd code && oxfmtformats the entire directory. While oxfmt supports file-specific formatting likeoxfmt src/file.tsoroxfmt '**/*.{ts,tsx}', the documentation doesn't show this option. Consider adding an example for contributors who only want to format changed files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 237, The docs currently show only the global command "cd code && oxfmt"; update AGENTS.md to document file-specific usage by adding examples and patterns (e.g., "oxfmt src/file.ts" and "oxfmt '**/*.{ts,tsx}'") and a short note about quoting/globbing for shells so contributors can run oxfmt against a single changed file or a glob of files instead of the whole directory.code/package.json (1)
46-53: Consider a catch-all stagedoxfmtentry.Oxfmt supports a much wider file set than these two patterns, and its lint-staged docs recommend a catch-all formatter entry. Keeping
oxfmtscoped here leaves those other supported file types out of the pre-commit auto-fix path even thoughyarn lint:fmtwill still process them in bulk. (oxc.rs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/package.json` around lines 46 - 53, The lint-staged config currently limits oxfmt to two glob patterns ("*.{js,jsx,mjs,ts,tsx}" and "*.{html,json}"), leaving other oxfmt-supported file types unhandled; update the package.json lint-staged entries to add a catch-all oxfmt entry (e.g., a glob like "*" or "**/*") so oxfmt runs on any staged file type it supports alongside the existing specific patterns, ensuring the oxfmt task name ("oxfmt") in the lint-staged object is used for the new catch-all entry and preserving the existing "yarn lint:js:cmd" associations for the specific globs.code/core/scripts/generate-source-files.ts (1)
69-77: Verify that theseformat()calls still honor the repo formatter config.The old explicit config plumbing is gone, and these generators now rely on bare
format(...)calls. Oxfmt’s docs only describe automatic.oxfmtrc.jsondiscovery for the CLI, while the Node API example configuresformat()via explicit options, so please confirm these outputs still pick up the repo settings; otherwise the generated files will churn againstyarn lint:fmt. (oxc.rs)Also applies to: 105-118, 167-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/scripts/generate-source-files.ts` around lines 69 - 77, The generators currently call format(...) without loading the repository's oxfmt config, which can cause churn; change each bare format(...) call (the ones producing versions.ts and the other blocks around lines 105-118 and 167-178) to first resolve the repo formatter config (using oxfmt’s Node API e.g., a load/findConfig or resolveConfig call) and pass that config into format(...) as options (with a sensible fallback if no config is found), so the formatted output honors the repo .oxfmtrc.json before writing with writeFile(destination, formatted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/package.json`:
- Around line 28-30: The package.json scripts currently run oxfmt in write mode
and omit the required lint:other script (ci expects yarn lint:other), so change
the scripts to make formatting validation check-only: update "lint:fmt" to run
"oxfmt --check" and add a new "lint:other" script that runs the remaining
validators used by CI (e.g., same commands as "lint" minus write-mode
formatting), ensuring "lint" composes "lint:js", "lint:ejs" and the new
"lint:other" so CI's yarn lint:other succeeds and oxfmt no longer mutates files
during CI.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 237: The docs currently show only the global command "cd code && oxfmt";
update AGENTS.md to document file-specific usage by adding examples and patterns
(e.g., "oxfmt src/file.ts" and "oxfmt '**/*.{ts,tsx}'") and a short note about
quoting/globbing for shells so contributors can run oxfmt against a single
changed file or a glob of files instead of the whole directory.
In `@code/core/scripts/generate-source-files.ts`:
- Around line 69-77: The generators currently call format(...) without loading
the repository's oxfmt config, which can cause churn; change each bare
format(...) call (the ones producing versions.ts and the other blocks around
lines 105-118 and 167-178) to first resolve the repo formatter config (using
oxfmt’s Node API e.g., a load/findConfig or resolveConfig call) and pass that
config into format(...) as options (with a sensible fallback if no config is
found), so the formatted output honors the repo .oxfmtrc.json before writing
with writeFile(destination, formatted).
In `@code/package.json`:
- Around line 46-53: The lint-staged config currently limits oxfmt to two glob
patterns ("*.{js,jsx,mjs,ts,tsx}" and "*.{html,json}"), leaving other
oxfmt-supported file types unhandled; update the package.json lint-staged
entries to add a catch-all oxfmt entry (e.g., a glob like "*" or "**/*") so
oxfmt runs on any staged file type it supports alongside the existing specific
patterns, ensuring the oxfmt task name ("oxfmt") in the lint-staged object is
used for the new catch-all entry and preserving the existing "yarn lint:js:cmd"
associations for the specific globs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 781bf779-1421-4074-b66a-16a684e17604
⛔ Files ignored due to path filters (2)
code/lib/codemod/src/transforms/__tests__/__snapshots__/upgrade-deprecated-types.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
.github/workflows/fork-checks.yml.oxfmtrc.json.serena/memories/project_overview.md.vscode/settings.jsonAGENTS.mdCONTRIBUTING.mdcode/.oxfmtrc.jsoncode/.prettierignorecode/addons/docs/docs/frameworks/WEB_COMPONENTS.mdcode/core/scripts/generate-source-files.tscode/core/src/common/utils/__tests-formatter__/withPrettierConfig/.prettierrccode/core/src/csf/story.tscode/core/src/manager-api/modules/stories.tscode/frameworks/angular/src/client/angular-beta/ComputesTemplateFromComponent.test.tscode/frameworks/angular/src/client/angular-beta/StorybookModule.test.tscode/frameworks/angular/src/client/csf-factories.test.tscode/frameworks/angular/src/client/decorateStory.test.tscode/frameworks/web-components-vite/template/cli/js/Header.jscode/frameworks/web-components-vite/template/cli/ts/Header.tscode/lib/codemod/src/transforms/__tests__/csf-2-to-3.test.tscode/lib/codemod/src/transforms/csf-2-to-3.tscode/lib/codemod/src/transforms/upgrade-deprecated-types.tscode/lib/eslint-plugin/scripts/update-lib-configs.tscode/lib/eslint-plugin/scripts/update-lib-flat-configs.tscode/lib/eslint-plugin/scripts/update-lib-index.tscode/lib/eslint-plugin/src/index.tscode/package.jsoncode/prettier.config.mjscode/renderers/web-components/src/csf-factories.test.tscode/renderers/web-components/src/docs/__testfixtures__/lit-element-demo-card/input.jscode/renderers/web-components/template/cli/js/Header.jscode/renderers/web-components/template/cli/ts/Header.tscode/renderers/web-components/template/stories/demo-wc-card/DemoWcCard.jsprettier.config.mjsscripts/.eslintrc.cjsscripts/build/utils/dts-process.tsscripts/create-nx-sandbox-projects.tsscripts/package.jsonscripts/prettier.config.jsscripts/utils/tools-esm.tsscripts/utils/tools.ts
💤 Files with no reviewable changes (7)
- scripts/utils/tools.ts
- scripts/.eslintrc.cjs
- code/prettier.config.mjs
- scripts/prettier.config.js
- prettier.config.mjs
- code/.prettierignore
- scripts/utils/tools-esm.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/utils/get-new-story-file.test.ts (1)
169-169: Avoid quote-style-coupled assertion here.This assertion is brittle to formatter/config changes. Match the import semantically so behavior is tested regardless of
'vs"output.Proposed change
- expect(storyFileContent).toContain('import { fn } from "storybook/test";'); + expect(storyFileContent).toMatch( + /import\s+\{\s*fn\s*\}\s+from\s+['"]storybook\/test['"];/ + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/get-new-story-file.test.ts` at line 169, The test's assertion is brittle because it expects a specific quote style; update the assertion on storyFileContent (the expect(...).toContain call) to match the import semantically using a quote-agnostic regex or pattern match (e.g., use toMatch with a regex that allows either ' or " and tolerates whitespace) so the test verifies the import of fn from storybook/test regardless of formatter quote changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/utils/get-new-story-file.test.ts`:
- Line 169: The test's assertion is brittle because it expects a specific quote
style; update the assertion on storyFileContent (the expect(...).toContain call)
to match the import semantically using a quote-agnostic regex or pattern match
(e.g., use toMatch with a regex that allows either ' or " and tolerates
whitespace) so the test verifies the import of fn from storybook/test regardless
of formatter quote changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0abc445f-04e5-46a4-8b57-dd035e16be77
📒 Files selected for processing (1)
code/core/src/core-server/utils/get-new-story-file.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/cli-storybook/src/automigrate/fixes/migrate-addon-console.test.ts (1)
18-25:⚠️ Potential issue | 🟠 MajorAdd
spy: trueoption and move mock implementation tobeforeEach; fix return type to Promise.The
formatFileContentmock at line 24 has multiple issues:
- Missing
spy: trueoption invi.mock()call- Mock behavior is inline in the module factory instead of in a
beforeEachblock- Critical: The real
formatFileContentis async (returnsPromise<string>), but the mock returns a synchronous string directlyApply these fixes:
- Add
spy: trueto thevi.mock()call- Move all mock implementations to a
beforeEachblock usingvi.mocked()- Wrap the mock return value in a Promise to match the async signature
Per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests", "Implement mock behaviors inbeforeEachblocks in Vitest tests", "Each mock implementation should return a Promise for async functions in Vitest", and "Avoid mock implementations outside ofbeforeEachblocks in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/cli-storybook/src/automigrate/fixes/migrate-addon-console.test.ts` around lines 18 - 25, Add the missing spy: true option to the vi.mock(...) call and remove inline implementations from the module factory; instead, in a beforeEach block use vi.mocked(getAddonNames), vi.mocked(removeAddon) and vi.mocked(formatFileContent) to set their mock implementations, ensuring formatFileContent returns a Promise<string> (e.g., Promise.resolve(content)) to match the real async signature; keep the module factory only returning the spread original exports with jest spies created by vi.mock(..., { spy: true }) and configure behavior in beforeEach.
🧹 Nitpick comments (2)
code/core/scripts/generate-source-files.ts (1)
69-79: Extract a sharedformatAndWritehelper to remove repetition.The same
format(...)+writeFile(...)sequence appears three times; a helper would centralize behavior and reduce drift risk.♻️ Proposed refactor
+async function formatAndWrite( + filename: string, + destination: string, + source: string +): Promise<void> { + const { code } = await format(filename, source, { singleQuote: true }); + await writeFile(destination, code); +} + async function generateVersionsFile(): Promise<void> { const destination = join(CORE_ROOT_DIR, 'src', 'common', 'versions.ts'); @@ - const { code: formatted } = await format( - 'versions.ts', - dedent` - // auto generated file, do not edit - export default ${versions}; - `, - { singleQuote: true } - ); - - await writeFile(destination, formatted); + await formatAndWrite( + 'versions.ts', + destination, + dedent` + // auto generated file, do not edit + export default ${versions}; + ` + ); } @@ - const { code: formatted } = await format( - 'frameworks.ts', - dedent` - // auto generated file, do not edit - export enum SupportedFramework { - // CORE - ${coreFrameworks}, - // COMMUNITY - ${communityFrameworks} - } - `, - { singleQuote: true } - ); - - await writeFile(destination, formatted); + await formatAndWrite( + 'frameworks.ts', + destination, + dedent` + // auto generated file, do not edit + export enum SupportedFramework { + // CORE + ${coreFrameworks}, + // COMMUNITY + ${communityFrameworks} + } + ` + ); } @@ - const { code: formatted } = await format( - 'exports.ts', - dedent` - // this file is generated by sourcefiles.ts - // this is done to prevent runtime dependencies from making it's way into the build/start script of the manager - // the manager builder needs to know which dependencies are 'globalized' in the ui - - export default ${JSON.stringify(data)} as const; - `, - { singleQuote: true } - ); - - await writeFile(destination, formatted); + await formatAndWrite( + 'exports.ts', + destination, + dedent` + // this file is generated by sourcefiles.ts + // this is done to prevent runtime dependencies from making it's way into the build/start script of the manager + // the manager builder needs to know which dependencies are 'globalized' in the ui + + export default ${JSON.stringify(data)} as const; + ` + ); }Also applies to: 106-121, 169-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/scripts/generate-source-files.ts` around lines 69 - 79, Extract a small helper named formatAndWrite (or similar) that takes (filename: string, source: string) and encapsulates the await format(...) call and subsequent await writeFile(destination, formatted) logic used in this file; replace the three duplicated blocks (the one that builds 'versions.ts' plus the other occurrences around lines 106-121 and 169-182) to call formatAndWrite('versions.ts', dedent`...`) (and the other filenames/contents) so formatting and file writing are centralized, returning the formatted result or void as needed and preserving the existing options ({ singleQuote: true }).code/.oxfmtrc.json (1)
10-11: Consider removing*.md/*.mdxfrom ignores once rollout is stable.Keeping docs ignored long-term will likely create formatting drift between code and docs; a tracked follow-up would help avoid that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/.oxfmtrc.json` around lines 10 - 11, The .oxfmtrc.json currently ignores "*.md" and "*.mdx" which will let docs diverge from code formatting; remove the "*.md" and "*.mdx" entries from the ignore list in .oxfmtrc.json so docs are formatted by the tool, and if you can’t flip this immediately open a tracked follow-up (issue/PR) to re-enable docs formatting once the rollout is stable so this change isn’t lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.test.ts`:
- Around line 9-11: The test currently sets a mock implementation at module
scope via vi.mocked(formatFileContent).mockImplementation(...); move that mock
behavior into a beforeEach block so the implementation is configured per-test
(e.g., add beforeEach(() =>
vi.mocked(formatFileContent).mockImplementation(async (_path, content) =>
content));) and ensure mocks are cleared between tests (use vi.resetAllMocks()
or vi.restoreAllMocks() in afterEach) so formatFileContent's mock does not leak
across tests; keep the vi.mock('storybook/internal/common', { spy: true }) at
module scope but relocate the mockImplementation for formatFileContent to
beforeEach and clean up in afterEach.
---
Outside diff comments:
In `@code/lib/cli-storybook/src/automigrate/fixes/migrate-addon-console.test.ts`:
- Around line 18-25: Add the missing spy: true option to the vi.mock(...) call
and remove inline implementations from the module factory; instead, in a
beforeEach block use vi.mocked(getAddonNames), vi.mocked(removeAddon) and
vi.mocked(formatFileContent) to set their mock implementations, ensuring
formatFileContent returns a Promise<string> (e.g., Promise.resolve(content)) to
match the real async signature; keep the module factory only returning the
spread original exports with jest spies created by vi.mock(..., { spy: true })
and configure behavior in beforeEach.
---
Nitpick comments:
In `@code/.oxfmtrc.json`:
- Around line 10-11: The .oxfmtrc.json currently ignores "*.md" and "*.mdx"
which will let docs diverge from code formatting; remove the "*.md" and "*.mdx"
entries from the ignore list in .oxfmtrc.json so docs are formatted by the tool,
and if you can’t flip this immediately open a tracked follow-up (issue/PR) to
re-enable docs formatting once the rollout is stable so this change isn’t lost.
In `@code/core/scripts/generate-source-files.ts`:
- Around line 69-79: Extract a small helper named formatAndWrite (or similar)
that takes (filename: string, source: string) and encapsulates the await
format(...) call and subsequent await writeFile(destination, formatted) logic
used in this file; replace the three duplicated blocks (the one that builds
'versions.ts' plus the other occurrences around lines 106-121 and 169-182) to
call formatAndWrite('versions.ts', dedent`...`) (and the other
filenames/contents) so formatting and file writing are centralized, returning
the formatted result or void as needed and preserving the existing options ({
singleQuote: true }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c47f1bc-a755-4026-81c4-9f7d9e3ccde6
⛔ Files ignored due to path filters (1)
code/lib/codemod/src/transforms/__tests__/__snapshots__/upgrade-deprecated-types.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
code/.oxfmtrc.jsoncode/core/scripts/generate-source-files.tscode/core/src/core-server/utils/get-new-story-file.test.tscode/lib/cli-storybook/src/automigrate/fixes/migrate-addon-console.test.tscode/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/codemod/src/transforms/__tests__/csf-2-to-3.test.tscode/lib/eslint-plugin/docs/rules/await-interactions.mdcode/lib/eslint-plugin/docs/rules/csf-component.mdcode/lib/eslint-plugin/docs/rules/default-exports.mdcode/lib/eslint-plugin/docs/rules/hierarchy-separator.mdcode/lib/eslint-plugin/docs/rules/meta-inline-properties.mdcode/lib/eslint-plugin/docs/rules/meta-satisfies-type.mdcode/lib/eslint-plugin/docs/rules/no-redundant-story-name.mdcode/lib/eslint-plugin/docs/rules/no-renderer-packages.mdcode/lib/eslint-plugin/docs/rules/no-stories-of.mdcode/lib/eslint-plugin/docs/rules/no-title-property-in-meta.mdcode/lib/eslint-plugin/docs/rules/no-uninstalled-addons.mdcode/lib/eslint-plugin/docs/rules/story-exports.mdcode/lib/eslint-plugin/docs/rules/use-storybook-expect.mdcode/lib/eslint-plugin/docs/rules/use-storybook-testing-library.mdcode/lib/eslint-plugin/scripts/update-lib-configs.tscode/lib/eslint-plugin/scripts/update-lib-flat-configs.tscode/lib/eslint-plugin/scripts/update-lib-index.tsdocs/configure/integration/eslint-plugin.mdx
✅ Files skipped from review due to trivial changes (11)
- code/lib/eslint-plugin/docs/rules/use-storybook-expect.md
- code/lib/eslint-plugin/docs/rules/no-uninstalled-addons.md
- code/lib/eslint-plugin/docs/rules/no-renderer-packages.md
- code/lib/eslint-plugin/docs/rules/use-storybook-testing-library.md
- code/lib/eslint-plugin/docs/rules/no-title-property-in-meta.md
- code/lib/eslint-plugin/docs/rules/no-stories-of.md
- code/lib/eslint-plugin/docs/rules/csf-component.md
- code/lib/eslint-plugin/docs/rules/meta-satisfies-type.md
- code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
- code/lib/eslint-plugin/docs/rules/story-exports.md
- code/lib/eslint-plugin/docs/rules/hierarchy-separator.md
🚧 Files skipped from review as they are similar to previous changes (3)
- code/lib/eslint-plugin/scripts/update-lib-configs.ts
- code/lib/eslint-plugin/scripts/update-lib-flat-configs.ts
- code/core/src/core-server/utils/get-new-story-file.test.ts
…ry.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Closes #
What I did
This PR migrate prettier to oxfmt for perf improvements. Tried to keep most of our former rules to avoid having a gigantic PR.
@kasperpeulen should we also migrate codemods ? This maybe better to migrate later as everyone's not on oxfmt yet. If so I can revert the commit.Same for scripts/tools ?
ℹ️ oxfmt also format mdx. This is an opportunity to re-enable mdx formatting in docs in another PR.
❓ should we also remove old prettier rules in a future PR to stick closer to oxlint rules from now on ?
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit