Implement loading of local flow-bin

[halbgut]

Fixes #24. This PR implements a check for a local version of flow-bin and uses that instead of the global one when found.

More specifically, it plimbs up the directory tree from the current working directory, checking there's a node_modules directory. If one is found it checks if [...]/node_modules/.bin/flow is a readable file. If that's the case, it sets g:flow#flowpath to that path. Otherwise it will still be set to flow.

[ghost]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[ghost]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[andreypopp]

This is useful, could this be merged?

[mroch]

probably a minor concern, but this could be a security risk. if you clone a malicious repo or a malicious npm package installs a binary called flow, you could end up running a binary that isn't Flow.

I think it's useful though. could this be turned on by a setting, so users have to opt into it?

[samwgoldman]

One could use finddir and filereadable as is done here in their own vim configuration, no? Maybe we should include this as an example in the README with a clear warning about the security risk?

[halbgut]

malicious npm package installs a binary called flow

There are far better attack vectors for malicious NPM package. Most systems (including my own) could be owned using a preinstall script without the user (or me :D) even noticing. This means an installed package can already execute code as the user in a far more efficient manner. So in my opinion there is no security gain from not loading node_modules/.bin/flow by default.

One could use finddir and filereadable as is done here in their own vim configuration, no?

The solution in this PR does have one big advantage. Installing Flow (project-) locally is best practice (for good reasons). This plugin as it stands encourages users to install it globally, which in most cases is the wrong choice.

I think it's useful though. could this be turned on by a setting, so users have to opt into it?

I think creating a setting sounds like a good idea. But in my opinion it should be opt out.

Edit:
P.S. Sorry for the late answer, I was quite badly under the weather for the last few days.

[tugorez]

Will this be merged?

[tugorez]

@Kriegslustig I've tried your solution and is not working.

I'm using Bundle, and vim 7.4.576 on Debian.

I'm not a vim expert (neither as user nor developer) but if found replacing

let s:npm_local_flowpath = finddir("node_modules", ".;") . "/.bin/flow"

with

let s:npm_local_flowpath = "./node_modules/.bin/flow"

gets the job done

If there is something I could help with, please tell me.

[halbgut]

@tugorez The issue with that solution is, that it will only work, if you're opening vim from from the project's root directory.

Could you run this command in vim and post the output here?

:echo finddir("node_modules", ".;") . "/.bin/flow"

Edit:
Please also check if :pwd gives you a directory that is either inside a project using flow or is the root directory of a project using flow.

[tugorez]

Oh I get it
Yes, for sure. It outputs

/.bin/flow 

Thank you @Kriegslustig.

[halbgut]

And running :!ls | grep node_modules in vim gives you node_modules?

[tugorez]

@Kriegslustig I see your point and you're right.
It was my problem because I ran the command in a non-dependencies-installed project, sorry.

[TrySound]

There's windows case which would be great to be handled.
https://github.com/prabirshrestha/asyncomplete-flow.vim/blob/master/autoload/asyncomplete/sources/flow.vim#L81-L87

[acornejo]

Is there something needed for this to be merged?

[loyd]

What is the problem w/ this PR? Let's merge.

[loyd]

I think, we need to detect flowpath for every .js file, not only at startup.

[halbgut]

Yeah good point. Though it'll hurt performance. I implemented it as you suggested.

[loyd]

Good job, thanks!

[fleischie]

@Kriegslustig I understand, that this PR is ancient time-wise. But could you shortly elaborate on why the function is named "SaveGetFlowExecutable?"

Is it, because it tries to find one in a specific manner and if it cannot find one, it returns with a warning? Shouldn't it then be "SafeGetFlowExecutable?" Also wouldn't make "s:local_flowpath" more sense, then "s:npm_local_flowpath," as the package manager has nothing to do with the path itself?

I am not trying to diss your PR, I am trying to understand it, because I'd like to merge it into my own personal fork.

Edit: I have some further amendmends if you're interested. If not, just let me know, then I'll take of it myself. 😄 ✌️