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

chore: use biome to format JSON and JavaScript files #8280

Closed
wants to merge 6 commits into from

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Aug 30, 2023

Changes

With this PR, I want to propose the usage of biome for format JSON and JS files instead of prettier.

Also, it proposed to remove the organize-imports-cli and use biome to do that.

Here's the benchmarks run on my machine.

Current format:ci command:

Benchmark 1: pnpm format:ci
  Time (abs ≡):        42.289 s               [User: 55.376 s, System: 7.550 s]

Format + organize imports of biome:

Benchmark 1: pnpm format:source
  Time (abs ≡):        404.7 ms               [User: 742.7 ms, System: 170.9 ms]

biome + prettier (we need to keep prettier to format yml, astro and md files):

Benchmark 1: pnpm format:ci
  Time (abs ≡):         2.402 s               [User: 2.658 s, System: 1.071 s]

This comes to a price though, having another tool inside the code base. Although, I think this is worth the price:

  • CI is going to be snappier and faster;
  • we removed organize-imports-cli, which was taking a lot of time to finish;
  • the action [ci] format is going to take way less time to apply the commit;

Testing

The current CI should pass.

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2023

⚠️ No Changeset found

Latest commit: a09ee5a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added feat: markdown Related to Markdown (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) pkg: example Related to an example package (scope) pkg: react Related to React (scope) pkg: preact Related to Preact (scope) pkg: solid Related to Solid (scope) pkg: lit Related to Lit (scope) pkg: integration Related to any renderer integration (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: astro Related to the core `astro` package (scope) labels Aug 30, 2023
@ematipico ematipico force-pushed the chore/use-biome-formatter branch 2 times, most recently from 7e5b975 to bd71c57 Compare August 30, 2023 10:13
@github-actions github-actions bot removed the pkg: solid Related to Solid (scope) label Aug 30, 2023
@ematipico ematipico force-pushed the chore/use-biome-formatter branch from bd71c57 to 6d35ba3 Compare August 30, 2023 10:49
@MrHBS
Copy link

MrHBS commented Aug 31, 2023

It would be fantastic if biome can support md, yml and Astro so we can get rid of prettier altogether :D

@ematipico ematipico force-pushed the chore/use-biome-formatter branch from 6d35ba3 to 8a7e4db Compare September 5, 2023 09:28
@ematipico ematipico marked this pull request as ready for review September 5, 2023 09:29
@ematipico ematipico requested a review from a team as a code owner September 5, 2023 09:29
@ematipico
Copy link
Member Author

ematipico commented Sep 5, 2023

It would be fantastic if biome can support md, yml and Astro so we can get rid of prettier altogether :D

It could, yes u.u

Unfortunately, we're not there yet.

@ematipico ematipico force-pushed the chore/use-biome-formatter branch from 8a7e4db to c8d1892 Compare September 5, 2023 09:31
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Skimming through the changes they look fine to me. Have one comment below about tabs in json. Other than that it looks great! We should also follow-up and remove the [ci] format commit so they formatting is enforced in CI.

biome.json Outdated Show resolved Hide resolved
@natemoo-re
Copy link
Member

I'm in favor of this! Could you add ad0d2b0 to .git-blame-ignore-revs?

@natemoo-re natemoo-re mentioned this pull request Sep 5, 2023
@natemoo-re
Copy link
Member

Just a heads up that I also opened #8420, but we can merge either one first

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Sep 7, 2023

we need to keep prettier to format yml, astro and md files

If you really want to be snazzy and fast for those files too, you could replace Prettier with dprint 😄 . And at that point you might as well replace Biome with dprint altogether, no?

A couple of interesting dprint references:

@MrHBS
Copy link

MrHBS commented Sep 7, 2023

@JoshuaKGoldberg Might as well use dprint for js/ts too 😁

Edit:

so perhaps dprint would be a better Prettier replacement altogether?

I somehow missed this…

@JoshuaKGoldberg
Copy link
Contributor

Ha, my phrasing was rushed and odd. Edited for clarity. Thanks 😄

@MrHBS
Copy link

MrHBS commented Sep 7, 2023

Hmm looks like dprint doesn’t have an official plugin for formatting yaml or Astro.

@dsherret
Copy link

dsherret commented Sep 7, 2023

Hmm looks like dprint doesn’t have an official plugin for formatting yaml or Astro.

It's possible to use prettier in dprint for that (https://github.com/dprint/dprint-plugin-prettier). It will run faster than prettier because it executes prettier on multiple threads.

@ematipico ematipico force-pushed the chore/use-biome-formatter branch from e318892 to 6a3ce6c Compare September 16, 2023 08:21
@ematipico
Copy link
Member Author

I'm in favor of this! Could you add ad0d2b0 to .git-blame-ignore-revs?

I wanted to do that, but should we wait for the PR to be merged? I thought that when we merge the PR, GitHub creates a new commit.

Comment on lines +7 to +8
# New formatting using Biome
07613b96bbe7a74179895c8cbe5f364133edd9ee
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we should only do this after merging. The commit hash will change

Comment on lines -1 to +3
import './server-shim.js';
import { LitElementRenderer } from '@lit-labs/ssr/lib/lit-element-renderer.js';
import * as parse5 from 'parse5';
import './server-shim.js';
Copy link
Member

Choose a reason for hiding this comment

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

I think we need the import './server-shim.js'; to be first as it's side-effectful and presumably affects how @lit-labs/ssr/lib/lit-element-renderer.js is loaded, so the order matters.

Copy link
Member

Choose a reason for hiding this comment

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

They should honestly be in separate group if the order is important, any formatter would make this mistake

Copy link
Member

Choose a reason for hiding this comment

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

Yeah putting a space between the imports would be fine by me too. I guess if biome wants to be more cautious about this automatically, plain import '...' ordering should be left as-is.

@jacobdalamb
Copy link
Contributor

Will biome also be added to the astro sites?

@Princesseuh
Copy link
Member

Will biome also be added to the astro sites?

No, this is only for our own repo.

@jacobdalamb
Copy link
Contributor

Why is this closed?

@Princesseuh
Copy link
Member

Why is this closed?

Since Biome can't format a fair amount of our files, it feels weird to have multiple formatters with different styles and configs

@ematipico
Copy link
Member Author

ematipico commented Oct 30, 2023

Why is this closed?

It seems there isn't enough interest and I don't want to keep the PR open. If someone else wants to open a new one, they are welcome

@bluwy bluwy deleted the chore/use-biome-formatter branch October 8, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) pkg: lit Related to Lit (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants