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.

Sign up for GitHub

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

Jump to bottom

feat: biome lint #4853

Merged
merged 5 commits into from
Sep 29, 2023
Merged

feat: biome lint #4853

merged 5 commits into from
Sep 29, 2023

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented Sep 27, 2023

This PR changes our linter/formatter to biome (https://biomejs.dev/)
Causing our prehook to run almost instantly, and our "yarn lint" task to run in sub 100ms.

Some trade-offs:

  • Biome isn't quite as well established as ESLint
  • Are we ready to install a different vscode plugin (the biome plugin) instead of the prettier plugin

biome results
ESLint results

The configuration set for biome also has a set of recommended rules, this is turned on by default, in order to get to something that was mergeable I have turned off a couple the rules we seemed to violate the most, that we also explicitly told eslint to ignore.

@chriswk chriswk self-assigned this Sep 27, 2023
@vercel
Copy link

vercel bot commented Sep 27, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Sep 29, 2023 5:23am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Sep 29, 2023 5:23am

@chriswk
Copy link
Contributor Author

chriswk commented Sep 27, 2023

If we're happy with this, we could eventually apply it to frontend as well.

@sighphyre
Copy link
Member

Tested locally. Uh.... wow.... that's fast

@sighphyre
Copy link
Member

Biome isn't quite as well established as ESLint
Are we ready to install a different vscode plugin (the biome plugin) instead of the prettier plugin

Both of these seem like really low risk, high value trade-offs to me.

@thomasheartman
Copy link
Contributor

I love the idea of this! But wait, it replaces both eslint and prettier? I thought it was just a linter. But I don't mind 😄

gastonfournier
gastonfournier reviewed Sep 28, 2023
View reviewed changes
Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

Overall, I like the recommendations, I love the speed boost, and I'm curious why frontend didn't get any recommendations.

I think this is a reversible decision (despite the many lines of code changed), so I'd in favor of trying it out

src/lib/error/from-legacy-error.ts Show resolved Hide resolved
src/lib/services/feature-toggle-service.ts Show resolved Hide resolved
src/lib/services/state-service.ts
@@ -633,7 +633,7 @@ export default class StateService {
async importTagTypes(
tagTypes: ITagType[],
keepExisting: boolean,
oldTagTypes: ITagType[] = [], // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe same comment as in the other parameter with a default value and // eslint-disable-line

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I'm very intrigued now... this was added by renovate bot! https://github.com/Unleash/unleash/pull/1147/files#diff-8757be4693afd352b1e0507343d2de88066e2d8ec2f453a11be93920b9d0b182L599

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's very odd.

@chriswk
Copy link
Contributor Author

chriswk commented Sep 28, 2023

I love the idea of this! But wait, it replaces both eslint and prettier? I thought it was just a linter. But I don't mind 😄

Yeah, reading the history of Rome from Meta, before it became biome, it seems like they're also planning on a jest compatible test runner, but the buy in for now is for formatter and linter, though using the config we can turn off parts

@chriswk
Copy link
Contributor Author

chriswk commented Sep 28, 2023

So, @gastonfournier frontend did not get any changes because the base setup in root folder only touches src folder, frontend has a separate setup in its package json. Different PR. Enough changes here already

@gastonfournier
Copy link
Contributor

So, @gastonfournier frontend did not get any changes because the base setup in root folder only touches src folder, frontend has a separate setup in its package json. Different PR. Enough changes here already

I can imagine! It crossed my mind that this could be a reason 👍

nunogois
nunogois approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

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

LGTM - Let's try it out!

@chriswk chriswk merged commit 6673d13 into main Sep 29, 2023
9 checks passed
@chriswk chriswk deleted the feat/biomeLint branch September 29, 2023 12:18
@nunogois nunogois mentioned this pull request Oct 2, 2023
Merged
nunogois added a commit that referenced this pull request Oct 2, 2023
@nunogois
feat: biome lint frontend (#4903)
4167a60 
Follows up on #4853 to add Biome
to the frontend as well.


![image](https://github.com/Unleash/unleash/assets/14320932/1906faf1-fc29-4172-a4d4-b2716d72cd65)

Added a few `biome-ignore` to speed up the process but we may want to
check and fix them in the future.
@mataha mataha mentioned this pull request Dec 10, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastonfournier gastonfournier gastonfournier approved these changes

@nunogois nunogois nunogois approved these changes

@sighphyre sighphyre Awaiting requested review from sighphyre

Assignees

@chriswk chriswk

Labels
None yet
Projects
Issues and PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chriswk @sighphyre @thomasheartman @gastonfournier @nunogois