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

🐛 Different formatting results in GitHub action #2109

Closed
1 task done
believer opened this issue Mar 16, 2024 · 10 comments · Fixed by #2186
Closed
1 task done

🐛 Different formatting results in GitHub action #2109

believer opened this issue Mar 16, 2024 · 10 comments · Fixed by #2186
Assignees
Labels
A-Core Area: core S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@believer
Copy link

Environment information

CLI:
  Version:                      1.6.1
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.19.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         unset

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

What happened?

When running biome in a GitHub action that uses ubuntu-latest we see errors in formatting that we don't see locally on macOS. If we change the runner to macos-latest everything works.

We're seeing the same behavior in GitHub and when running the actions locally using act. We've also tried things like changing Node versions and removing caches.

This just started happening without any configuration changes on our part, so unsure what caused it.

jobs:
  lint:
    name: Lint and formatting
    # Switching to macos-latest makes it work
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-node@v4
        with:
          node-version-file: '.nvmrc'
      - run: corepack enable
      - run: yarn install
      - run: yarn biome ci .

Using ubuntu-latest we are seeing errors like the ones below.

// Trailing comma in type generics
 File content differs from formatting output

16 16    }
17 17 
18     - export·const·entity·=·<T>(data:·T,·links:·(e:·T)·=>·Links):·Entity<T>·=>·{
   18  + export·const·entity·=·<T,>(data:·T,·links:·(e:·T)·=>·Links):·Entity<T>·=>·{
19 19      return {
20 20        data,
····· 
23 23}
24 24 
25     - export·const·entityList·=·<T>(
   25  + export·const·entityList·=·<T,>(
26 26      list: T[],
27 27      listLinks: () => Links,
// Ternary formatting
113      - ······{title
114      - ········?·<View·className="border-stroke-variant2·border-b·px-4·pt-6·pb-2">
115      - ············<T.Label·size="medium"·testID="radio-group-title">
116      - ··············{title}
117      - ············</T.Label>
118      - ··········</View>
119      - ········:·null}
    113  + ······{title·?·(
    114  + ········<View·className="border-stroke-variant2·border-b·px-4·pt-6·pb-2">
    115  + ··········<T.Label·size="medium"·testID="radio-group-title">
    116  + ············{title}
    117  + ··········</T.Label>
    118  + ········</View>
    119  + ······)·:·null}
// Object keys
30 30          compact: 'px-1 py-0.5',
31 31          default: 'px-2 py-1',
32     - ······'8':·'p-2',
33     - ······'12':·'p-3',
34     - ······'16':·'p-4',
35     - ······'24':·'p-6',
36     - ······'32':·'p-8',
   32  + ······8:·'p-2',
   33  + ······12:·'p-3',
   34  + ······16:·'p-4',
   35  + ······24:·'p-6',
   36  + ······32:·'p-8',
37 37        },
38 38        bordered: {

Expected result

Formatting result should be the same on both runners

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@ematipico
Copy link
Member

Probably a duplicate of #2080. Can you confirm?

@believer
Copy link
Author

Yes, it seems to be related to that issue! Drilling down a couple of levels, for example biome ci ./apps/api, makes the error disappear.

@Sec-ant
Copy link
Member

Sec-ant commented Mar 22, 2024

Is there any chance you can provide a minimal repro on this issue? You can try only keeping the files with the unstable code blocks and keeping their paths/folder structure untouched.

@believer
Copy link
Author

@Sec-ant I'll see if I can put one together

@believer
Copy link
Author

@Sec-ant Here's a minimal repro which mimics the layout of our setup. The biome.json is identical, except for name changes, to what we're using.

Repro: https://github.com/believer/biome-2109
Failing workflow: https://github.com/believer/biome-2109/actions/runs/8387734624/job/22970506971

I noticed something that might be a clue. biome ci . works fine when I only have the files inside the apps folder. But, once I add components.ts inside libs/ui/components/src, it give me the ternary formatting error from above in test.tsx inside apps/app/src.

If I remove components.ts, the error disappears.

@Sec-ant
Copy link
Member

Sec-ant commented Mar 22, 2024

@believer Thank you! I'm able to reproduce it on my laptop now (Ubuntu WSL). I'll take a look into this one.

image


Update, there're some race conditions in this issue:

image

@Sec-ant
Copy link
Member

Sec-ant commented Mar 23, 2024

This and #2080 should be the same problem. But I'll leave this open because we also have some info here.

I did some debug yesterday and I think the reason of this issue is that we'are caching the JsFormatOptions too aggressively. We have jsx and standard variants for js files and these two variants will treat < and > symbols differently when parsing and formatting. Our caching strategy now will treat all js files as one of these two variants during traversal. If we treat them all as jsx files, the result will be fine, but if we treat them all as standard js files, the formatting result will be altered.

As we are traversing all the files simultaneously, which variant will be cached is not deterministic, so we can find some race conditions on this issue. This also explains why using a nested path or adding/removing standard js files in the traversal tree will alter the results.

cc @ematipico

@ematipico
Copy link
Member

This is the PR that introduced some level of caching: #1944

The creation of a data structure for re using the same file source among files that have the same extension. It's possible that this caching needs to be smarter.

Alternatively, if a solution can't be found quickly, we could decide to remove it for now.

@Sec-ant
Copy link
Member

Sec-ant commented Mar 23, 2024

@ematipico If you don't mind, I want to take a further look into it in these two days. Is that ok?

@ematipico
Copy link
Member

Of course! Thank you for the help

@ematipico ematipico added A-Core Area: core S-Bug-confirmed Status: report has been confirmed as a valid bug labels Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Area: core S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants