Skip to content

Conversation

yesudeep
Copy link
Contributor

@yesudeep yesudeep commented Feb 14, 2025

fix(js): test on node versions 20 through 22; 23 is currently broken #1995

ISSUE: #1995

CHANGELOG:

  • Update biome configuration to use arrow function arg parens always.
  • Update github workflows to test on node versions 20 through 22.
  • We will add version 23 post fixes.
  • Set node 22 to be the default for our eng workstations in the
    setup script for now.
    REFERENCES:

The configuration for the formatting is more or less based on the Google
TypeScript formatting guidelines at:

Checklist (if applicable):

@yesudeep yesudeep marked this pull request as draft February 14, 2025 17:06
@yesudeep yesudeep force-pushed the yesudeep/fix/biome branch 4 times, most recently from fa677b9 to 7aab58c Compare February 14, 2025 17:39
@yesudeep yesudeep changed the title [WIP] Use biome for formatting faster. [WIP] fix(js): format TypeScript files using the faster biomejs formatter and Google style #1987 Feb 14, 2025
@yesudeep yesudeep self-assigned this Feb 14, 2025
@yesudeep yesudeep force-pushed the yesudeep/fix/biome branch 8 times, most recently from 0194928 to 7bfd67f Compare February 15, 2025 04:06
@yesudeep yesudeep changed the title [WIP] fix(js): format TypeScript files using the faster biomejs formatter and Google style #1987 fix(js): test on node 20-22; format TS with biomejs #1987 #1995 Feb 15, 2025
@yesudeep yesudeep mentioned this pull request Feb 15, 2025
5 tasks
@yesudeep yesudeep requested review from pavelgj and mbleigh February 15, 2025 04:22
@yesudeep yesudeep marked this pull request as ready for review February 15, 2025 04:40
Copy link
Collaborator

@mbleigh mbleigh left a comment

Choose a reason for hiding this comment

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

One comment on Node versions to test, but I'll leave to @pavelgj to decide if we want to do this for sure.

@MichaelDoyle
Copy link
Member

Maybe you all already discussed, and I am an outlier here... but... can we separate out the toolchain changes from the style changes into separate PRs?

Some of the changes in formatting seem good. But others seem superfluous, and some even go against the style guide. Particularly, removing parens around all of the arrow functions goes against:

Parentheses around the left-hand side of a single-argument arrow function are recommended but not required.

And is a huge part of the overall diff here.

@pavelgj
Copy link
Collaborator

pavelgj commented Feb 15, 2025

Maybe you all already discussed, and I am an outlier here... but... can we separate out the toolchain changes from the style changes into separate PRs?

Some of the changes in formatting seem good. But others seem superfluous, and some even go against the style guide. Particularly, removing parens around all of the arrow functions goes against:

Parentheses around the left-hand side of a single-argument arrow function are recommended but not required.

And is a huge part of the overall diff here.

I'm a little caught by surprise here....
I'm personally pretty happy with prettier and not sure we need to change it.

@yesudeep yesudeep force-pushed the yesudeep/fix/biome branch 3 times, most recently from ca47587 to 8b41fb4 Compare February 15, 2025 22:19
@yesudeep
Copy link
Contributor Author

Maybe you all already discussed, and I am an outlier here... but... can we separate out the toolchain changes from the style changes into separate PRs?

Some of the changes in formatting seem good. But others seem superfluous, and some even go against the style guide. Particularly, removing parens around all of the arrow functions goes against:

Parentheses around the left-hand side of a single-argument arrow function are recommended but not required.

And is a huge part of the overall diff here.

Fixed. I'll separate the toolchain changes from the formatting for easier review as well.

@yesudeep
Copy link
Contributor Author

yesudeep commented Feb 15, 2025

Maybe you all already discussed, and I am an outlier here... but... can we separate out the toolchain changes from the style changes into separate PRs?
Some of the changes in formatting seem good. But others seem superfluous, and some even go against the style guide. Particularly, removing parens around all of the arrow functions goes against:

Parentheses around the left-hand side of a single-argument arrow function are recommended but not required.

And is a huge part of the overall diff here.

I'm a little caught by surprise here.... I'm personally pretty happy with prettier and not sure we need to change it.

4x-12x for our codebase is a significant speed up with no loss of functionality.

zsh❯ hyperfine -i "pnpm run format:biome" "pnpm run format:prettier"
Benchmark 1: pnpm run format:biome
  Time (mean ± σ):     453.6 ms ±   5.6 ms    [User: 679.3 ms, System: 92.5 ms]
  Range (min … max):   447.9 ms … 465.3 ms    10 runs

  Warning: Ignoring non-zero exit code.

Benchmark 2: pnpm run format:prettier
  Time (mean ± σ):      5.715 s ±  0.285 s    [User: 9.578 s, System: 0.739 s]
  Range (min … max):    5.506 s …  6.426 s    10 runs

Summary
  pnpm run format:biome ran
   12.60 ± 0.65 times faster than pnpm run format:prettier

I'll keep prettier in this PR and continue the discussion on [discord]

@yesudeep yesudeep changed the title fix(js): test on node 20-22; format TS with biomejs #1987 #1995 fix(js): test on node versions 20 through 22; 23 is currently broken #1995 Feb 15, 2025
@yesudeep
Copy link
Contributor Author

How do I trigger the pending CI checks? It looks like they are stalled.

…1995

ISSUE: #1995

CHANGELOG:
- [x] Update biome configuration to use arrow function arg parens always.
- [x] Update github workflows to test on node versions 20 through 22.
- [x] We will add version #23 post fixes.
- [x] Set node 22 to be the default for our eng workstations in the
  setup script for now.

The configuration for the formatting is more or less based on the Google
TypeScript formatting guidelines at:

- [x]  https://google.github.io/styleguide/tsguide.html#string-literals
- [x]  https://google.github.io/styleguide/jsguide.html
- [x]  https://google.github.io/styleguide/tsguide.html#arrow-function-bodies
- [x]  https://google.github.io/styleguide/tsguide.html#automatic-semicolon-insertion
@yesudeep yesudeep closed this Feb 17, 2025
@yesudeep yesudeep deleted the yesudeep/fix/biome branch April 9, 2025 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants