Flow-for-vscode: Wrong flow binary/version used with multi-root vscode workspaces

Created on 19 Jan 2018  Â·  6Comments  Â·  Source: flowtype/flow-for-vscode

When multi-root workspaces are used in vscode, this extension will use the same flow binary for all packages, even if useNPMPackagedFlow is enabled and the workspace folders have different flow versions defined in their package.json files.

This results in a mismatch between the flow errors you see reported in VSCode and the errors that are reported by running flow from the command-line with npx flow. In addition, when you run flow from the command-line, flow catches the version mismatch, kills the old server, and starts a new server. This results in wasted time waiting for flow to initialize over and over.

This happens for two reasons:

  1. The extension doesn't look in each workspace folder for the flow binaries. It currently uses the deprecated workspace.rootPath property instead of workspace.workspaceFolders[].uri, so it only gets the flow binary for the first folder in the workspace. I expect, though I have not experienced this, that if the first folder in the workspace didn't have flow installed, the extension would fail to find the flow binary even if other folders in the workspace had it installed). See: https://github.com/flowtype/flow-for-vscode/blob/dceceae29dc8ccb495b92660fe84746f4f5839ee/lib/pkg/flow-base/lib/FlowHelpers.js#L187
  2. The extension caches the path to the flow binary, but it caches it for the entire workspace and not for each folder in that workspace. There needs to be a cache for each folder. See: https://github.com/flowtype/flow-for-vscode/blob/dceceae29dc8ccb495b92660fe84746f4f5839ee/lib/pkg/flow-base/lib/FlowHelpers.js#L161

Most helpful comment

For me it seems that the extension is not multi-root workspace capable at all. Only the very first contained project is used, with some strange results:

If I do not have a flow enabled project as first project, the extension will even fail to locate a local flow installation and tries to resort to a global flow.

I think the task should be extended to make it multi-root compatible and not just fix the local flow lookup. There might be other places that silently fail or just use the wrong data.

All 6 comments

I'm seeing this too. I hope to submit a patch soon.

Actually, my plan is to, for each file, look for the nearest package.json which has flow-bin declared as a (dev) dependency, and use that (fallback to the one at workspace root / global) (and then cache the result per directory).

For our use-case, vscode multi-root workspaces look like a pain and this is all we needed them for anyway.

@spudly would that work for you as well?

Yes, i think so.

On Mon, Jan 29, 2018, 4:14 PM Alex Rattray (Stripe Acct) <
[email protected]> wrote:

Actually, my plan is to, for each file, look for the nearest package.json
which has flow-bin declared as a (dev) dependency, and use that (fallback
to the one at workspace root / global) (and then cache the result per
directory).

For our use-case, vscode multi-root workspaces look like a pain and this
is all we needed them for anyway.

@spudly https://github.com/spudly would that work for you as well?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/flowtype/flow-for-vscode/issues/201#issuecomment-361388593,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAdunJK4RH2cIFZm4th9HkusFL8pcGFPks5tPjTGgaJpZM4RkbUt
.

For me it seems that the extension is not multi-root workspace capable at all. Only the very first contained project is used, with some strange results:

If I do not have a flow enabled project as first project, the extension will even fail to locate a local flow installation and tries to resort to a global flow.

I think the task should be extended to make it multi-root compatible and not just fix the local flow lookup. There might be other places that silently fail or just use the wrong data.

@rattrayalex-stripe did you get anywhere with this? Would be really handy!

Sorry, no. I've been hoping the bigger LSP changes would pave the groundwork for this, if not include the fix directly.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

vojtatranta picture vojtatranta  Â·  4Comments

Eugene-Musika-cpamatica picture Eugene-Musika-cpamatica  Â·  6Comments

micmro picture micmro  Â·  4Comments

jpokrzyk picture jpokrzyk  Â·  6Comments

siegebell picture siegebell  Â·  5Comments