Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

Support automatically checking for the node_modules version of Flow #53

Merged
merged 4 commits into from
Dec 6, 2016
Merged

Support automatically checking for the node_modules version of Flow #53

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e84acc8
Support automatically falling back to the node_modules version of flo…
orta Nov 23, 2016
66bd3a5
Use a flag around local flow versions
orta Dec 2, 2016
e2642ac
Cache existing flow paths
orta Dec 4, 2016
a529b32
Merge master
orta Dec 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions lib/pkg/flow-base/lib/FlowHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function isOptional(param: string): boolean {
}

async function isFlowInstalled(): Promise<boolean> {
const flowPath = getPathToFlow();
const flowPath = await getPathToFlow();
if (!flowPathCache.has(flowPath)) {
flowPathCache.set(flowPath, await canFindFlow(flowPath));
}
Expand All @@ -144,12 +144,32 @@ async function canFindFlow(flowPath: string): Promise<boolean> {
}

/**
* @return The path to Flow on the user's machine. It is recommended not to cache the result of this
* function in case the user updates his or her preferences in Atom, in which case the return
* value will be stale.
* @return The path to Flow on the user's machine. First using the the user's config, then looking into
* the node_modules for the project.
* It is recommended not to cache the result of this function in case the user updates his or her
* preferences in Atom, in which case the return value will be stale.
*/
function getPathToFlow(): string {
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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it does, maybe the logic should be switched around realistically


if (await canFindFlow(userPath)) {
return userPath
}

const shouldUseNodeModule = config.get('useNPMPackagedFlow')
if (shouldUseNodeModule && await canFindFlow(fallbackNodeModuleFlowLocation())){
return fallbackNodeModuleFlowLocation()
}
return userPath
}

/**
* @return The potential path to Flow on the user's machine if they are using NPM/Yarn to manage
* their installs of flow.
*/
function fallbackNodeModuleFlowLocation(): string {
return global.vscode.workspace.rootPath + "/node_modules/.bin/flow"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought that for windows support I would need to use path.join but Stack Overflow says it's not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - this came up with my Jest work - I will need to do this

}

function getStopFlowOnExit(): boolean {
Expand Down
4 changes: 2 additions & 2 deletions lib/pkg/flow-base/lib/FlowProcess.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export class FlowProcess {

/** Starts a Flow server in the current root */
async _startFlowServer(): Promise<void> {
const pathToFlow = getPathToFlow();
const pathToFlow = await getPathToFlow();
// `flow server` will start a server in the foreground. asyncExecute
// will not resolve the promise until the process exits, which in this
// case is never. We need to use spawn directly to get access to the
Expand Down Expand Up @@ -342,7 +342,7 @@ export class FlowProcess {
...args,
'--from', 'nuclide',
];
const pathToFlow = getPathToFlow();
const pathToFlow = await getPathToFlow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much slower do you think this call is? I mainly worry about things like autocomplete, where adding 100ms can really degrade performance.

Copy link
Contributor Author

@orta orta Dec 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the difference is a fileExists check for which I don't expect to be a slow it's a which 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

const ret = await asyncExecute(pathToFlow, args, options);
if (ret.exitCode !== 0) {
// TODO: bubble up the exit code via return value instead
Expand Down
10 changes: 4 additions & 6 deletions lib/utils/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,11 @@

import spawn from 'cross-spawn';
import {window, workspace} from 'vscode'
import {getPathToFlow} from "../pkg/flow-base/lib/FlowHelpers"

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')
}

export function isFlowEnabled() {
return workspace.getConfiguration('flow').get('enabled')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this would have different behaviour now, I made them share the code instead

}
Expand All @@ -36,9 +33,10 @@ export function checkNode() {
}
}

export function checkFlow() {
export async function checkFlow() {
const path = await getPathToFlow()
try {
const check = spawn(process.platform === 'win32' ? 'where' : 'which', [getPathToFlow()])
const check = spawn(process.platform === 'win32' ? 'where' : 'which', [path])
let
flowOutput = "",
flowOutputError = ""
Expand Down
7 changes: 6 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@
"type": "boolean",
"default": true,
"description": "Stop Flow on Exit"
},
"flow.useNPMPackagedFlow": {
"type": "boolean",
"default": false,
"description": "Support using flow through your node_modules folder, WARNING: Checking this box is a security risk. When you open a project we will immediately run code contained within it."
}
}
}
Expand Down Expand Up @@ -77,4 +82,4 @@
"bugs": {
"url": "https://github.com/flowtype/flow-for-vscode/issues"
}
}
}