Skip to content
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

Replace Prettier with Biome #7441

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pjonsson
Copy link
Contributor

@pjonsson pjonsson commented Jan 4, 2025

What this PR does

The Prettier challenge was a reward
of $20 000 for a formatter written
in Rust that passes 95% of the Prettier
test suite. Biome won that:

https://prettier.io/blog/2023/11/27/20k-bounty-was-claimed

This PR does a "yarn biome migrate prettier"
and tweaks the result so the formatting
differences are minimal to Prettier 2.7.1
that is currently used. The performance
of Prettier 3.4.2 is comparable to 2.7.1,
and Biome is quite a bit faster than both
of them:

$ time -p yarn prettier
...
wwwroot/test/Terria/applyInitData/MapServer/mapServerWithError.json 1ms
Done in 19.41s.
real 19.54
user 28.86
sys 2.23

$ time -p yarn format
yarn run v1.22.22
$ biome format --write .
Formatted 1376 files in 251ms. Fixed 70 files.
Done in 0.35s.
real 0.49
user 1.83
sys 0.27

For testing purposes, this PR has put the formatting changes in a separate commit so that can be dropped and the branch be rebased against latest main without merge conflicts. The formatting commit should be squashed into the first commit when this is about to get merged.

Test me

Run yarn format, especially on other platforms than Linux.

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • [] I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

The Prettier challenge was a reward
of $20 000 for a formatter written
in Rust that passes 95% of the Prettier
test suite. Biome won that:

https://prettier.io/blog/2023/11/27/20k-bounty-was-claimed

This PR does a "yarn biome migrate prettier"
and tweaks the result so the formatting
differences are minimal to Prettier 2.7.1
that is currently used. The performance
of Prettier 3.4.2 is comparable to 2.7.1,
and Biome is quite a bit faster than both
of them:

$ time -p yarn prettier
...
wwwroot/test/Terria/applyInitData/MapServer/mapServerWithError.json 1ms
Done in 19.41s.
real 19.54
user 28.86
sys 2.23

$ time -p yarn format
yarn run v1.22.22
$ biome format --write .
Formatted 1376 files in 251ms. Fixed 70 files.
Done in 0.35s.
real 0.49
user 1.83
sys 0.27
: [H] extends [true]
? true
: false;
? true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The formatting of ?: is consistent with Prettier version 3.4.2.

@@ -146,8 +146,7 @@ class MapboxVectorTileCatalogItem extends MappableMixin(
}
}

@computed
/** Convert traits into paint rules:
@computed /** Convert traits into paint rules:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prettier 3.4.2 also has this problem. Split out the change that moves the JSDoc to #7440 so that can get merged now without having to update the formatter.

@@ -190,7 +190,10 @@ export default class WebMapServiceCapabilities {
readonly [name: string]: CapabilitiesLayer;
};

private constructor(readonly xml: XMLDocument, readonly json: any) {
private constructor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't appreciate that constructors with readonly parameters are split over multiple lines even when they fit with room to spare in a single line, but this formatting is also consistent with Prettier 3.4.2.

"**/*.scss.d.ts",
"./wwwroot/third_party/*",
"doc/acknowledgements/attributions.md",
"./wwwroot/test/init/sdmx-abs.json"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a 1.7 MB file that hasn't been updated since 2017, so put it in the ignore list to not slow down the formatting that is done.

@pjonsson
Copy link
Contributor Author

pjonsson commented Jan 4, 2025

I changed from black + isort + flake8 to Ruff in another project, and while I've known that faster is always better, I underestimated the impact of the speedup on my general mood during development. It's a life changing experience when formatting/linting is instant. Trying to quantify instant: it takes between 0.1 and 0.4s to blink, so the formatting is almost completed if you blink after pressing enter to run the formatter.

Despite my praise for instant formatting/linting, I think the outstanding big PRs (Webpack 5, TypeScript 5.7, React imports, TSification) should be merged before this PR to avoid needless merge conflicts.

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.

1 participant