Skip to content

Remove prettier prod dependency in favor of user-installed formatter#1639

Closed
bluwy wants to merge 3 commits intonextfrom
format-option
Closed

Remove prettier prod dependency in favor of user-installed formatter#1639
bluwy wants to merge 3 commits intonextfrom
format-option

Conversation

@bluwy
Copy link
Copy Markdown
Member

@bluwy bluwy commented Apr 17, 2025

Uses https://github.com/JoshuaKGoldberg/formatly to auto-detect the project's formatter for formatting the changesets and changelogs. Introduces a new format: "auto" | "prettier" | "biome" | "deno" | "dprint" | false option to control the formatting, and removes the prettier: boolean option in favor of this new option.

Tests has also been updated. Because test fixtures are inside /tmp directories with no access to prettier, the test assertions are now all against un-formatted code and has been updated accordingly.


NOTES:

  1. On prettier option removal: I didn't deprecate since it was introduced v2.28.0 (17 Feb 2025, not long ago), so I don't expect its usage to be widespread enough to need to maintain it for the next major, so I removed it directly.
  2. On formatting failure: I can't seem to replicate cases where formatting fails, so I didn't add any warning/error logs for now, but could be something to follow-up if weird things can happen there.
  3. On formatly being new: I think it has potential to be a bit more robust but it should relieve us from maintenance. One caveat I notice is that since it calls a child process, it can take around 300-800ms after confirming the changeset in the CLI.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 17, 2025

🦋 Changeset detected

Latest commit: 237e280

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@changesets/apply-release-plan Major
@changesets/config Major
@changesets/types Major
@changesets/cli Major
@changesets/write Minor
@changesets/assemble-release-plan Patch
@changesets/get-release-plan Patch
@changesets/changelog-git Patch
@changesets/changelog-github Patch
@changesets/get-dependents-graph Patch
@changesets/parse Patch
@changesets/pre Patch
@changesets/read Patch
@changesets/release-utils Patch
@changesets/should-skip-package Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR


const prettierInstance =
options?.prettier !== false ? getPrettierInstance(cwd) : undefined;
const formatter = await getFormatter(options?.format ?? "auto", cwd);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how many of the supported formatters support formatting .md files?

Copy link
Copy Markdown
Member Author

@bluwy bluwy Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch. Biome actually doesn't support it. The rest (prettier, deno, dprint) do. When detecting the formatter, should we prefer prettier in this case if both biome and prettier exist?

formatly currently detects and choose biome first, so we might need to workaround or fix that upstream 🤔

Another thought, if biome supports markdown in the future, do we unblock detecting biome within a semver minor/patch? Feels a bit of a gray area. EDIT: I think in practice it's ok in a minor, since if it affects anything it's only the dev workflow.

if (!formatter) return async () => {};

return async (filePath: string) => {
await formatly([filePath], { cwd, formatter });
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One caveat I notice is that since it calls a child process, it can take around 300-800ms after confirming the changeset in the CLI.

Filed: JoshuaKGoldberg/formatly#114

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JoshuaKGoldberg/formatly#144 should fix the performance issue. Do you have appetite to try that out for this PR?

I'm hesitant to merge the PR until we can confirm it works here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOLO, merged it 🚀 . Available as formatly@0.3.0.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.19%. Comparing base (771dc07) to head (237e280).
⚠️ Report is 107 commits behind head on next.

Files with missing lines Patch % Lines
packages/apply-release-plan/src/index.ts 77.27% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1639      +/-   ##
==========================================
- Coverage   72.25%   72.19%   -0.06%     
==========================================
  Files          65       65              
  Lines        5013     4999      -14     
  Branches      874      874              
==========================================
- Hits         3622     3609      -13     
+ Misses       1386     1385       -1     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bluwy
Copy link
Copy Markdown
Member Author

bluwy commented May 4, 2026

I'm going to close this as it's probably easier to start over

@bluwy bluwy closed this May 4, 2026
@beeequeue
Copy link
Copy Markdown

beeequeue commented May 4, 2026

but using formatly is probably the way to do it at least, it seems like a very nice solution

@beeequeue beeequeue mentioned this pull request May 4, 2026
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants