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

🐛 biome ignores shared biome config in git hook commands in monorepo #47

Closed
1 task done
sbsrnt opened this issue Mar 26, 2024 · 7 comments
Closed
1 task done

Comments

@sbsrnt
Copy link

sbsrnt commented Mar 26, 2024

Edit: Referencing this issue for visibility in biomejs/biome#2228 in order to help track down any related issues should this be resolved/supported.


Environment information

CLI:
  Version:                      1.6.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.9.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.1.0"

Biome Configuration:
  Status:                       unset

Workspace:
  Open Documents:               0

What happened?

It's a little bit hard to replicate my setup with the playground links due to the shared config package and ci nature.

Example run in ci: https://github.com/exile-watch/nucleus/actions/runs/8441346324/job/23120305017#step:2:252

Here is the shared config: https://github.com/exile-watch/splinters/blob/main/packages/biome-config/biome.json#L30-L34

Here is how it's utilized: https://github.com/exile-watch/nucleus/blob/main/packages/encounter-data/biome.json

And here is a package json that has tabs instead of spaces: https://github.com/exile-watch/nucleus/blob/main/packages/encounter-data/package.json


Locally everything is passing when i go with npm run format or npm run lint but for some reason the ci command fails


Another potential issue I've noticed, though this one is with pre-commit git hooks, is that the recipe command in biomejs.dev site is resetting any format/lint --apply changes (which also gives an indication of somewhat ignoring some rules from shared config)

pre-commit:
  commands:
    check:
      glob: "*.{js,ts,cjs,mjs,d.cts,d.mts,jsx,tsx,json,jsonc}"
      run: npx biome check --no-errors-on-unmatched --files-ignore-unknown=true {staged_files}

this one in particular, but I can "easily" bypass that by changing the run script to npm run lint so it can respect my shared config

Expected result

I wouldn't expect indent errors to appear in ci.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Sec-ant
Copy link
Member

Sec-ant commented Mar 27, 2024

re the first issue:

I'm not getting it. The shared config suggests spaces should be used for indentation and biome ci catches the errors in package.json where you use tabs for indentation. Isn't this expected?

@sbsrnt
Copy link
Author

sbsrnt commented Mar 27, 2024

Right... Then it really boils down to pre-commit git hook npx @biomejs/biome check --no-errors-on-unmatched --files-ignore-unknown=true {staged_files} ignoring the formatter settings 🤔

And now when I'm testing it, I'm even more confused:
So here is a commit that passes the spacing indentation: exile-watch/nucleus@d6ba161

I'm not really sure why the {root}/package.json has tab indentation while other files are using spaces.

Here is a commit that fails the biome check as pre-commit hook removed manual format script call: exile-watch/nucleus@2906070.


Reproduction
  1. If you want to test it locally, you can create a {root}/lefthook.yml file with this content:
remotes:
  - git_url: https://github.com/exile-watch/splinters
    configs:
      - packages/lefthook-config/lefthook.yml
  1. In package.json add:
"format": "npx @biomejs/biome format --write .",
"lint:apply": "npx @biomejs/biome check --apply ."`
  1. Add shared biome config npm i -D @exile-watch/biome-config and update biome.json
{
  "extends": ["@exile-watch/biome-config"]
}
  1. Add:
  1. In both cases from step 4 there is a need to run npx lefthook install to sync the git hooks after installing the package

  2. Run npm run format

  3. Commit

  • 0.5.10 - working fine after commit as it is using npm run lint:apply
  • 0.5.11 - resets formatter changes after commit due to npx @biomejs/biome check --no-errors-on-unmatched --files-ignore-unknown=true {staged_files}

@exile-watch/biome-config includes biome dependency, @exile-watch/lefthook-config includes lefthook dependency

@sbsrnt
Copy link
Author

sbsrnt commented Mar 27, 2024

Now that I'm re-reading the reproduction steps... I think I know what is happening 🤔

pre-commit hooks are working from the repo root, so what I think is happening is, biome is looking for the {root}/biome.json, not {root}/packages/{package}/biome.json thus the pre-commit run command runs... the defaults 🤔?

@Sec-ant
Copy link
Member

Sec-ant commented Mar 27, 2024

Just typing it 😂:
image

@sbsrnt
Copy link
Author

sbsrnt commented Mar 27, 2024

I saw biomejs/biome#2080 the other day so waited for 1.6.3.

Based on the comments it looks like there is a common misconception that I think should be mentioned in the docs.

I will allow myself to summon @ematipico due to this comment: biomejs/biome#2080 (comment)

The guide explains to have a command in each package, NOT in the root. You can't do both, which is what you're trying to do.

This sentence is quite concerning because that indicates that with the setup above you can't have biome (that respects your rules) in git hooks - am I reading it correctly?

And if so, contrary to eslint/prettier setups, biome should be treated similar to, let's say lefthook, where we only have 1 global biome config and packages inside monorepo are expanding from that {root}/biome.json

@sbsrnt sbsrnt changed the title 🐛 Does biome ci ignore shared config formatter rules? 🐛 biome ignores shared biome config in git hook commands in monorepo Mar 27, 2024
@sbsrnt
Copy link
Author

sbsrnt commented Mar 27, 2024

To address my comment, should my understanding be correct:

Based on the comments it looks like there is a common misconception that I think should be mentioned in the docs

If I could propose a change (happy to contribute), it would involve splitting the big projects page into two categories: one for projects that don't have git hooks (as per the current version of the docs) and one for those that do. For this, we would need to make a small change in the docs to reflect the tree structure.

Current tree:

.
├─ backend
│   ├─ biome.json
│   └─ package.json
└─ frontend
     ├─ biome.json
     ├─ legacy-app
     │   └─ package.json
     └─ new-app
          └─ package.json

Proposed tree for category with git hooks:

.
├─ .git/hooks // add this dir
├─ biome.json // add this file
├─ backend
│   ├─ biome.json
│   └─ package.json
└─ frontend
     ├─ biome.json
     ├─ legacy-app
     │   └─ package.json
     └─ new-app
          └─ package.json

@castarco
Copy link

castarco commented Apr 5, 2024

pre-commit hooks are working from the repo root, so what I think is happening is, biome is looking for the {root}/biome.json, not {root}/packages/{package}/biome.json thus the pre-commit run command runs... the defaults 🤔?

It is exactly this. There are some related discussions on the topic, like biomejs/biome#1573 .

It seems that it isn't entirely trivial to solve given the current code structure (due to potential performance tradeoffs, among other details), so it needs some thinking and discussion.

@ematipico ematipico transferred this issue from biomejs/biome Apr 16, 2024
@ematipico ematipico closed this as not planned Won't fix, can't repro, duplicate, stale May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants