Support automatically checking for the node_modules version of Flow #53
Conversation
…w if the config flow cannot be found
| * their installs of flow. | ||
| */ | ||
| function fallbackNodeModuleFlowLocation(): string { | ||
| return global.vscode.workspace.rootPath + "/node_modules/.bin/flow" |
There was a problem hiding this comment.
I initially thought that for windows support I would need to use path.join but Stack Overflow says it's not needed.
There was a problem hiding this comment.
Have you tested this on Windows? I think this would work, but not near a Windows computer so can't double check. I vaguely remember that there's a node_modules/.bin/flow.cmd which is what you can use from the commandline, but I wonder if there is a .bin/flow or if the flow.cmd just references a binary inside of node_modules/flow
There was a problem hiding this comment.
Yeah - this came up with my Jest work - I will need to do this
| if (await canFindFlow(fallbackNodeModuleFlowLocation())){ | ||
| return fallbackNodeModuleFlowLocation() | ||
| } | ||
| return config |
There was a problem hiding this comment.
opted to return the config value here just in case ( and because the type is not optional )
|
|
||
| const NODE_NOT_FOUND = '[Flow] Cannot find node in PATH. The simpliest way to resolve it is install node globally' | ||
| const FLOW_NOT_FOUND = '[Flow] Cannot find flow in PATH. Try to install it by npm install flow-bin -g' | ||
|
|
||
| function getPathToFlow(): string { | ||
| return workspace.getConfiguration('flow').get('pathToFlow') | ||
| } |
There was a problem hiding this comment.
As this would have different behaviour now, I made them share the code instead
|
poke? |
|
Looks like this was discussed here: facebookarchive/nuclide#570 I can move this behind a flag? |
|
Alright, I've added it behind a flag - and offered the same warning as Nuclides 👍 |
| return global.vscode.workspace.getConfiguration('flow').get('pathToFlow') | ||
| async function getPathToFlow(): Promise<string> { | ||
| const config = global.vscode.workspace.getConfiguration('flow') | ||
| const userPath = config.get('pathToFlow') |
There was a problem hiding this comment.
Does this default to flow if not explicitly set? Does this mean that if flow is in the path, then the useNPMPackagedFlow option is ignored?
There was a problem hiding this comment.
yeah, it does, maybe the logic should be switched around realistically
| @@ -342,7 +342,7 @@ export class FlowProcess { | |||
| ...args, | |||
| '--from', 'nuclide', | |||
| ]; | |||
| const pathToFlow = getPathToFlow(); | |||
| const pathToFlow = await getPathToFlow(); | |||
There was a problem hiding this comment.
How much slower do you think this call is? I mainly worry about things like autocomplete, where adding 100ms can really degrade performance.
There was a problem hiding this comment.
the difference is a it's a fileExists check for which I don't expect to be a slowwhich lookup, so it would be - however, I can instead opt to cache the values, I think it's pretty easy to get a callback from VS Code when your settings changes and so the stored path can be reset then - will take a look
| * their installs of flow. | ||
| */ | ||
| function fallbackNodeModuleFlowLocation(): string { | ||
| return global.vscode.workspace.rootPath + "/node_modules/.bin/flow" |
There was a problem hiding this comment.
Have you tested this on Windows? I think this would work, but not near a Windows computer so can't double check. I vaguely remember that there's a node_modules/.bin/flow.cmd which is what you can use from the commandline, but I wonder if there is a .bin/flow or if the flow.cmd just references a binary inside of node_modules/flow
|
Alright - I've got it caching the path for flow, so you'll only get the hit of checking for I've also fixed the windows support I think |
|
And, well, I remembered to push to my fork too 💃 |
| @@ -1,50 +1,56 @@ | |||
| /* @flow */ | |||
There was a problem hiding this comment.
ah, bah, git was complaining about line ends - didn't think it would do this
|
This is rebased etc, and should be 👍 for review |
|
Sweet! This looks good to me! In the future we might need to be more aggressive with clearing the cache, but this looks good for now! Thanks for bearing with me! |
Addresses #41 by proxy too.
We have multiple apps that use different versions of Flow, which makes it tough to keep VS Code in a good state. We couldn't use an absolute path, or per-project settings (as they wouldn't work per user) and so I think this is a good fallback.