fix(engine): only start one engine for umbrella apps

[katafrakt]

This adds check for being in an umbrella while determining the project the file belongs to. The detection mechanism is finding mix.exs with apps_path defined in project, which is the best I was able to come up with without actually evaluating mix.exs.

When going up in search of umbrella mix.exs, we put a boundary on the root of the currently open project, so opening individual apps separately is still possible (and we don't go spelunking through the filesystem).

fixes #460

[mhanberg]

I'm kind of confused why so much specific code is required for this.

Shouldn't the way to determine the project is by comparing to the root_uri/workspace folder?

In Next LS that is how I dispatched requests/notifications to each engine node/document store

[katafrakt]

Shouldn't the way to determine the project is by comparing to the root_uri/workspace folder?

There might be no mix project at this. This is quite common for monorepos actually.

[mhanberg]

Shouldn't the way to determine the project is by comparing to the root_uri/workspace folder?

There might be no mix project at this. This is quite common for monorepos actually.

That is not what an umbrella app is tho. An umbrella app will have a project. And to clarify, i meant at the root of the workspace folder.

If someone opens a mono repo with 1 umbrella and 1 normal, you still just need to treat the umbrella as just an app

monorepo
- umbrella
    - mix.exs 
    - apps
        - child1
            - mix.exs
        - child2
            - mix.exs     
- normal
    - mix.exs

So for any file you check

# pseudo code, there would be more around absolute paths/etc
for project_path <- ["monorepo/umbrella", "monorepo/normal"] do
    String.starts_with?(file_uri, project_path)
end

[mhanberg]

Basically, the editor tells us the root directories of each workspace (folder) and we need to route requests to them based on the file uri. I don't think there is much else to it.

[katafrakt]

I'm opening my work monorepo now and it just sends one workspace folder:

"workspaceFolders":[{"uri":"file:///Users/pawel.sw/jump/Jump","name":"~/jump/Jump/"}]

The Elixir project is at ~/jump/Jump/api.

[mhanberg]

I'm opening my work monorepo now and it just sends one workspace folder:

"workspaceFolders":[{"uri":"file:///Users/pawel.sw/jump/Jump","name":"~/jump/Jump/"}]

The Elixir project is at ~/jump/Jump/api.

You have to tell the editor the workspace folders, it doesn't automatically guess them.

[katafrakt]

The thing is that I don't have to do it anymore. With automatic subprojects detection I just open one workspace folder and Expert finds a mix project in a subfolder.

[mhanberg]

The thing is that I don't have to do it anymore. With automatic subprojects detection I just open one workspace folder and Expert finds a mix project in a subfolder.

I think there may be confusion as to what we are trying to fix here.

Are you implying we already have that? I am not under the impression that we do. That is what workspace folders are for.

[doorgan]

What happens today:

• you start your editor at the umbrella root, and an engine is started for the umbrella
• you open a file in a subapp, and an engine is started for the sub-app too

We're using mix.exs as a proxy for "mix project", when you open a file we walk the document uri upwards until we find a .exs file, and we create a project for that.
In an umbrella project, if you open a file in a subapp, we will find the .exs file for that sub-app, start an engine for it, and route all requests to that project, never to the umbrella engine

[katafrakt]

yes, we have it I think since #18

[doorgan]

Yes, and that is what this PR is trying to fix. If we know you're at an umbrella, we'd need to start an engine for the umbrella root only.
I haven't extensively tested umbrellas bafere or after #18 so I'm not sure if having a single engine would affect completions. For example subapp A depends on subapp B, so A should get completions from B but B should not get completions from A

[mhanberg]

In my opinion, we need to assume the root of the workspace folder is the root of the app with the mix.exs file and start the engine in there.

[katafrakt]

I'm not sure if having a single engine would affect completions. For example subapp A depends on subapp B, so A should get completions from B but B should not get completions from A

Yeah, great catch. One engine for the whole umbrella means shared indexer, so actually B gets completions from A. It actually works as you described (A gets completions from B, but B does not from A) currently, without the changes from this PR - and with starting a separate engine for every subapp. I think ElixirSense fallback simply resolves B as a dependency of A and that's why the completion works.

[Draggu]

In my opinion, we need to assume the root of the workspace folder is the root of the app with the mix.exs file and start the engine in there.

If we assume the opened workspace folder is the project root, we’re effectively saying that editor defines project boundaries. That works when the user opens “the right” folder, but it can easily fail if they open a subapp in an umbrella, a single package in a monorepo, or just a random file. In those cases, things may not work correctly unless the workspace is set up exactly right.

We should treat mix.exs as the source of truth. As @doorgan said:

We're using mix.exs as a proxy for "mix project", when you open a file we walk the document uri upwards until we find a .exs file, and we create a project for that.

That way, project boundaries come from the actual project structure, not from the folders opened in the IDE. mix.exs defines what a project is, and we should rely on it.

LSP provides workspace folders, but it’s a broad, language-agnostic protocol. Except for document and configuration lifecycle events, we don’t have to care about anything else it provides if we don’t find it useful.

@katafrakt It would be helpful to document that umbrella apps now run a single engine, and more importantly, explain the reasoning behind this approach.

[mhanberg]

In my opinion, we need to assume the root of the workspace folder is the root of the app with the mix.exs file and start the engine in there.

If we assume the opened workspace folder is the project root, we’re effectively saying that editor defines project boundaries. That works when the user opens “the right” folder, but it can easily fail if they open a subapp in an umbrella, a single package in a monorepo, or just a random file. In those cases, things may not work correctly unless the workspace is set up exactly right.

We should treat mix.exs as the source of truth. As @doorgan said:

We're using mix.exs as a proxy for "mix project", when you open a file we walk the document uri upwards until we find a .exs file, and we create a project for that.

That way, project boundaries come from the actual project structure, not from the folders opened in the IDE. mix.exs defines what a project is, and we should rely on it.

LSP provides workspace folders, but it’s a broad, language-agnostic protocol. Except for document and configuration lifecycle events, we don’t have to care about anything else it provides if we don’t find it useful.

@katafrakt It would be helpful to document that umbrella apps now run a single engine, and more importantly, explain the reasoning behind this approach.

The editor is literally in charge of determining the root of a project, that is what workspace folders are and is what we need to use.

Umbrella apps are not mono repos and should not be used as such. I dont want to have a ton of code designed to determine if we should start another engine node or not, that is determined by the workspace folders.

I also don't want add logic for determining if they opened a dependency from inside the deps folder. If this is how we determine this now, then that explains why expert has been initializing new engines when i source dive my dependencies. we should not be starting new engines for thow.

Let us follow the protocol.

Also, the mix.exs file is an elixir script and not static data, determining what is inside it will be fraught with bugs as people often write dynamic code in there.

[Draggu]

The editor determines the workspace, but a workspace is not necessarily the same thing as a project. A client may not support workspace folders at all, so we cannot rely on that concept. If there’s no mix.exs, it’s not really an elixir project. It’s just a bunch of files.

I agree that umbrella apps are not monorepos and shouldn’t be treated as such. But if we place an umbrella app inside a monorepo alongside other non-elixir projects, the opened workspace is again not equal to the Elixir project itself. I also don’t want to add logic for determining if someone opened a dependency from inside the deps folder, and I don’t think there’s a need to check it.

The project structure should be obtained from mix, not manually.

None of this conflicts with the protocol.