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

Drop ESLint and Prettier for Biome #1848

Merged

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented May 15, 2024

Q A
Bug fix? no
New feature? no
Issues Fix #...
License MIT

Hi everyone,

Biome is a new modern tool for code linting and formatting.
It supports TypeScript out-of-the-box, lint and format does not conflict each-other (like ESLint and Prettier can do), and it's super fast!
With Biome, we can easily replace:

  • ESLint
  • ESLint's TypeScript plugin
  • Prettier
  • ESLint's Prettier plugin

There is a lot of modifications, but 99% of them are:

  • use import type ... / import { type ... } when necessary
  • removing 'use strict', since we have type: 'module' in package.json files, but I'm not 100% confidente here and I may add them back
  • using template string interpolation instead of + operator

I've disabled the following linting rules in order to reduce the number of modifications, but later we can start to enable them:

  • complexity/noStaticOnlyClass
  • complexity/noForEach
  • style/noParameterAssign
  • performance/noDelete

The yarn scripts lint, format, check-lint and check-format have been modified in consequences.

For performance comparisons, you can check those two CI jobs:

For the number of commits, I wanted to ease the review by doing many atomic commits, but feel free to squash if necessary.

WDYT?

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label May 15, 2024
@Kocal
Copy link
Member Author

Kocal commented May 15, 2024

Failures are unrelated

@smnandre
Copy link
Member

I cannot resist to answer with: https://dayssincelastjavascriptframework.com/

(and now more seriously i'll look at your post 😅 )

@smnandre
Copy link
Member

(first read: you know how to speak to me :) )

@Kocal
Copy link
Member Author

Kocal commented May 15, 2024

Yeah I know about the 0 day since the last JS framework/tools meme... and trust me it's not for nothing my TN is "JS hate account" :trollface:

Biome is a community-fork from Rome (started 3 years ago), it's a tool I was really looking forward to because of the over-fragmentation of JavaScript tools and the unnecessarily complex configurations to make them work together.
I love having a tool that does a lot, but mostly well. ... <troll>Next PR, Bun to replace Yarn/Vitest/Rollup? 👀 </troll>)

I consider Biome enough "production-ready", to nicely and quickly lint and format our JS(X)/TS(X) files.

@WebMamba
Copy link
Collaborator

What about editor integration? Did you tried the PHPstorm extension? Do you think it's mature enough?

@Kocal
Copy link
Member Author

Kocal commented May 15, 2024

I didn't know an extension existed, and so I didn't try it. But I'm not a big fan of ESLint/Prettier integration in IDEs, I prefer relying on CLI / CI / pre-commit hook :D

@smnandre
Copy link
Member

I'll try it next week (late in the week) !

src/Svelte/composer.json Outdated Show resolved Hide resolved
@@ -57,7 +55,7 @@ export default class extends Controller<HTMLInputElement> {
button.classList.add(...this.buttonClassesValue);
button.setAttribute('tabindex', '-1');
button.addEventListener('click', this.toggle.bind(this));
button.innerHTML = this.visibleIcon + ' ' + this.visibleLabelValue;
button.innerHTML = `${this.visibleIcon} ${this.visibleLabelValue}`;
Copy link
Member

Choose a reason for hiding this comment

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

Is this worth the change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, string concatenation with + is a mistake

"workspaces": [
"src/*/assets"
],
"workspaces": ["src/*/assets"],
"scripts": {
"build": "node bin/build_javascript.js && node bin/build_styles.js",
"test": "bin/run-vitest-all.sh",
Copy link
Member

Choose a reason for hiding this comment

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

Any impact on those two ? (build / test)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmmh nope, it is not related to Biome and they still works

@Kocal Kocal force-pushed the chore/replace-eslint-and-prettier-by-biomejs branch from e65d382 to 97dd32d Compare May 17, 2024 08:29
@Kocal Kocal force-pushed the chore/replace-eslint-and-prettier-by-biomejs branch from 97dd32d to 79953e3 Compare May 28, 2024 07:07
@Kocal Kocal force-pushed the chore/replace-eslint-and-prettier-by-biomejs branch from 79953e3 to a999295 Compare June 25, 2024 05:50
@javiereguiluz
Copy link
Member

This looks like a nice addition. Do you think this is ready for merge or would you like to wait for more feedback and/or changes? Thanks!

@Kocal
Copy link
Member Author

Kocal commented Jul 30, 2024

Hey, I think this PR would need a last rebase on main before merging.
I will do it this afternoon

@Kocal Kocal force-pushed the chore/replace-eslint-and-prettier-by-biomejs branch from a999295 to ffa391b Compare July 30, 2024 12:48
@Kocal
Copy link
Member Author

Kocal commented Jul 30, 2024

The PR has been rebased and CI checks are passing, feel free to merge it if you like it 👀

@javiereguiluz
Copy link
Member

Let's merge this now so we can fully test it for real in the project and see how it goes.

Thanks Hugo!

@javiereguiluz javiereguiluz force-pushed the chore/replace-eslint-and-prettier-by-biomejs branch from 2d41195 to b406997 Compare July 30, 2024 16:00
@javiereguiluz javiereguiluz merged commit 3732365 into symfony:2.x Jul 30, 2024
7 of 8 checks passed
@Kocal Kocal deleted the chore/replace-eslint-and-prettier-by-biomejs branch July 30, 2024 17:37
javiereguiluz added a commit that referenced this pull request Jul 30, 2024
…rn workspaces (Kocal)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

imp(biomejs): upgrade Biomejs, fix patterns, don't use yarn workspaces

| Q             | A
| ------------- | ---
| Bug fix?      | yes/no
| New feature?  | yes/no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

Following #1848:
- It looks like glob pattern like `src/*/{src,test}` (the `{a,b}` part) were not working
- I've configured files to lint/format in our `biome.json`.
- I've also upgraded Biomejs to ^1.8.3
- I've rerun Biome lint/format on the whole codebase

Sorry for the big PR 🙏🏻

Commits
-------

304bfae imp(biomejs): upgrade Biomejs, fix patterns, don't use yarn workspaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants