-
-
Notifications
You must be signed in to change notification settings - Fork 47
feat: search workspace sub directories for biome binary #718
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
base: main
Are you sure you want to change the base?
Conversation
Hey, thanks for the PR — this is a solid starting point. Before we move forward, there are a few concerns we need to address. Up until now, we've assumed that Biome is installed at the root of the workspace folder. This allows us to start a single LSP session that can lint and format all supported files within that folder. In other words, we’ve been dealing with one LSP session that handles all supported files in the workspace folder. Introducing support for locating Biome in sub-folders raises some important questions:
|
@nhedger You're asking some hard questions 😂 I don't mean to overstep here. I apologize in advance if I do.
Yes, this should be possible. Why? Consider how you are ALREADY supporting Multi-root workspaces:
source: https://biomejs.dev/reference/vscode/ It is my understanding after reviewing the code that each workspace does have its own biome binary using the version set by the workspace. In fact my implementation originally included looking for ALL biome.json files. However, I ended up only grabbing the first one I could find. It should actually do a thorough search and spin up multiple LSP sessions.
That's a tricky one! Basically what do you do if a parent repository already uses biome. Actually, you are already dealing with this exact issue. Why? You load the global instance after loading the workspace instance(s): src/extensions.ts:231 private async createInstances(): Promise<void> {
if (this.mode === "single-file") {
await this.createSingleFileInstance();
} else if (this.mode === "single-root" || this.mode === "multi-root") {
await this.createWorkspaceInstances();
}
await this.createGlobalInstance();
this.logger.info(`🚀 Created ${this.biomes.size} Biome instance(s).`);
} What do you do in this case? In my experience it is easy to forget about globally installed dependencies and to not update them for a long time, all the while keeping the dependency in the project updated. So what happens currently if the global instance would format it one way and the workspace instance would format it in another? Please correct me if I'm wrong, but I believe that if there was a decision made on how to handle global vs workspace instance should apply to root vs sub-directory instance. If no decision was made here and the behavior is currently unknown, I am happy to help the team find a solution, if you wish to include me in this process 🙂 |
Note I haven't read the whole proposal
I don't think we should allow that, and I believe the extension(s) should do as little as possible to avoid degrading the DX across editors. While the proposal is sound, we would enter in all sorts of problems, which aren't handled in core:
One important thing that Biome wants to deliver, is a good DX across editors using LSP, and reduce maintenance burden. If the extension starts doing things that core might be able to do, we increase the maintenance burden for developers. |
@ematipico I understand what you're saying. I agree that the extension should stay as simple as possible. Honestly, it is its simplicity that made me install biome in the first place. However, please be aware that you are already supporting multiple versions with your "multi-root workspaces" feature:
You might say that it is not the same and you are right. It's not the same on paper. But at the end of the day the DX is exactly the same. Yes, the developer chose to do this, but having multiple biome installations within a repo is also something a developer would have to choose! It doesn't happen by accident. You can neither enforce nor prevent this. Also please consider that you load a global instance ON TOP of the workspace instance! What happens when those two versions are not in sync? So currently you are basically already supporting all the things that you say will cause problems! If you firmly believe in what you just said, the "multi-root workspaces" feature should be deprecated/removed in my opinion and the global instance should only ever be loaded if there is no workspace instance. The can of worms is already open, even if you might not realize it yet. edit: fixed the grammar |
P.S: I do not mean to have an argument here and I might misunderstand things. I apologize in advance if that's the case. I just feel like the project might already be headed down a path that it was not intended for. It's surprisingly easy to add a new feature without fully understanding the implications (ask me how I know .. ). From the questions that were raised and the comment that was shared, I believe that you might not fully realize the implications of some of the built in features of this extension. (Or maybe I just made a fool of myself. I hope I did not 😂 ) |
@fabianszabo I think your argument makes sense. It may not be ideal, but the reality is indeed that for monorepos, we may discover situations where different Biome versions are installed in different packages. For the best UX, we should support that. And you're right, for multi-root workspaces, we have already implemented it to some extent. But that doesn't mean that going the extra mile to implement multiple versions within a single root is necessarily easy... And of course @ematipico's argument isn't that we should not support multiple versions within a root, but rather that if possible we should support it in Biome's core rather than in the extension. That way, all extensions can benefit and we avoid reinventing the wheel, and thus, maintenance cost. There's just a practical problem at the moment: I don't think we currently have the infrastructure to handle this in our core. But I do agree solving it there would be ideal. @fabianszabo Do you think you would be able to investigate what it would take to implement such infrastructure in Biome's core? |
@arendjr Thank you for your explanation! Here's a discussion thread: biomejs/biome#6966 |
Summary
Now searching for biome binary in sub directories.
Description
I tried to keep the implementation minimal but safe. It is reusing existing functions to find the binary, stops at a directory depth of 3 and skips "problematic" directories.
The way it finds the binary is by looking for the biome.json config. I think that this file is technically not required, but I believe that most people would have a config file and it makes the search simple and fast.
Related Issue
This PR closes #622 and #680
Checklist