Use flow-language-server behind a flag

[thymikee]

Summary

This PR aims to rewrite the plugin to use latest flow-language-server, implementing Language Server Protocol.

It may take a while, because I'm not sure if I find enough time to make it, but hey, at least I started!

Fixes #148

cc @wbinnssmith

TODO:

• Minimal working setup
• Diagnostics
• Suggestions/Autocomplete
• Status
• Coverage
• Available through config flag

[wbinnssmith]

Wow! That was fast :)

This looks really awesome. Some things that are currently missing from the language server wrapper that would be necessary for parity with this extension today.

We should decide what we're comfortable with to launch. I'd be understanding if that means parity with the existing extension.

• Coverage (which LSP API should this use? may need to get something into the protocol to support this first)
• Support for anonymous buffers as part of the project (currently implemented here with the sandbox)
• config options
  • flow.runOnAllFiles
  • flow.runOnEdit
    • this functionality is still flaky in Flow itself and I believe will be superseded by the ide option providing push diagnostics, though the language server will need to push buffer contents to Flow. cc @nmote
  • flow.runOnAllFiles
    • Right now diagnostics are implemented as this extension conditionally checking files with the type checker. I don't know if this should be implemented here because of the move to push diagnostics (see abouve). cc @nmote
  • flow.stopFlowOnExit
    • Needs to be implemented in flow-language-server. Currently it leaves Flow running 😳

[rattrayalex-stripe]

Is there any update here?

[thymikee]

@rattrayalex unfortunately not yet, didn't have time to play with it lately 🙁

[arahansen]

I'm interested in looking into this. But something that would help me jumping in would be understanding what benefits we are able to get out of this refactor.

Is there new functionality with flow-language-server? performance improvements? etc?

[thymikee]

The benefit is smaller cost of maintenance, because lots of implementation details are handled by the language server. The drawback is the lack of proper docs on how to interact with the APIs. As for tha functionality, I think there’s still some work to do on Flow sever side

[jwickens]

Does this work? I tried the 'selfhost' mentioned in contributing but couldnt get it to work. The current offering for vscode flow extensions all have several major issues, would be nice to get this published somewhere so people can try it.

[thymikee]

@jwickens works, but is quite limited currently. I'd treat this PR as a base for something better to come (still no bandwidth to tackle this, but here's corresponding issue #148)

[baransu]

I would really like to help with this PR but I'm missing knowledge about VSCode, its extensions and LSP. It would be helpful to get more info on what's need to be done as well as some guidance so I can contribute to this transition.

[thymikee]

@baransu huh, I'm also a newbie vscode extension dev here 😅, just wanted to play around, see if it's worth migrating (and since we could offload a lot to official flow team, it's worth it). I think you can start with official vscode docs and play around with some tutorial.

Then see example of language server: https://code.visualstudio.com/docs/extensions/example-language-server (what we need here is a language client, server is implemented by Flow team)

As for LSP, here are official docs https://github.com/Microsoft/language-server-protocol but I didn't find them helpful to be honest (maybe it was lack of time to get through it thoroughly?). A lot of the work done is by quick trial and error and looking up others work.

What I found helpful however is e.g. ESLint plugin for vscode, which is also implemented with LSP: https://github.com/Microsoft/vscode-eslint

Feel free to build on what I have here or start from scratch, whichever is easier.

[levrik]

@thymikee Specification of LSP can be found here: https://microsoft.github.io/language-server-protocol/specification

[jbachhardie]

I'm trying to build a flow-languageserver vscode integration from scratch so I can learn the quirks of it without getting confused by pre-existing stuff: https://github.com/jbachhardie/vscode-flow-ls

It currently runs and reports (if not very reliably) so if anyone else wants a green field sort of environment to test stuff out you can clone it.

[jslauthor]

@orta @thymikee 👋

Webflow just made an initial donation to see this PR come to fruition. We are excited for the flow language server so the flow community can achieve the same tooling as TypeScript.

@thymikee mentioned we could set a "bounty" on certain PRs. Consider #150 our target. 😄

Let us know if there's anything we can do to help move this forward! Thanks.

[thymikee]

Well, now I don't have any excuses to not work on this (just need to reprioritize OSS work for a while) 😅. Thanks @jslauthor for pushing this!
I'm more motivated by the Webflow's drive to get the plugin better, than the money itself. Also, almost everybody at Callstack is using it daily, so we have shared interest to make it better.

As far as the development goes, the idea is to get this feature behind the flag and improve it incrementally until we get feature parity, then switch to LSP by default. I'm happy to get the initial step done and then I hope we'll be able to split the tasks to smaller ones, so anybody interested in getting this done could take a share out of Webflow's generous donation.

[rcjsuen]

I've made a flow-lsp branch in my clone that changes the extension to delegate to flow-language-server for the existing IntelliSense, hovers, definition lookups, and diagnostics reporting features. I can make a separate pull request for the commit if this is desired.

I've only tested it with a very simple JavaScript file.

// @flow
function square(n: string): number {
    // uncomment and Ctrl+Space here for string functions
    // n.
    return 5;
}

square(true); // Error!

class Animal {

    talk(): void {

    }

    eat(): boolean {
        return false;
    }
}

let myInstance: Animal = new Animal();
myInstance.eat();
myInstance.talk();
// uncomment and Ctrl+Space here
// myInstance.

Sometimes things are super laggy, I have no idea why. I'm pretty sure 8 seconds is too long. I don't know what other people's experiences have been in their own experiments.

�[32m[2018-03-17 08:42:07.779] [INFO] nuclide-commons/process - �[39m7856ms: C\:\\Users\\XXX\\AppData\\Roaming\\Flow\\bin\\0.68.0\\flow.exe autocomplete --json c\:\\Users\\XXX\\test\\flow\\test.js 4 7 --retry-if-init false --retries 0 --no-auto-start --from nuclide

[rcjsuen]

From some more investigating, I realized that I can't drop lib/flowDiagnostics.js because the coverage coverage code directly depends on it for it to work.

We can't inspect the response for textDocument/publishDiagnostics in VS Code and route it through to Coverage first either because that's not currently supported (see microsoft/vscode-languageserver-node#299).

[rvion]

so far, all lsp-based forks I've seen do not work with multiple flow sub-projects (not multi-root, just several folders with .flowconfig). It should't be hard to support, and vscode has some examples about how to do this.

I'm commenting here now to say I really hope this extension won't switch to some newer lsp based implementation until the one root but multiple flow subfolders workflow is supported, because all my projects depends on it :)

thanks everyone making this great plugin.

[rcjsuen]

@rvion There are three ways to help with that I think.

1. Try out Atom's ide-flowtype and see if that works. If it does, then the language server should support this. If it doesn't, it sounds like this is something the language server needs to implement.

2. Try out the vscode-flow-ls extension by @jbachhardie and see if that works. Same as above. If it does then work towards this pull request should support it also. If it doesn't, then again, it sounds like this is something that needs to be looked at at the language server level.

3. Create a simple project on GitHub that replicates your project's folder structures and so on so people working on and testing out the LSP work can easily validate that your use case will be satisfied.

[jbachhardie]

Thing is I'm pretty sure Flow itself doesn't support running multiple instances of its server simultaneously so even if you did manage to get the multiple .flowconfig setup to work you're going to be paying a high performance cost of restarting the typechecking server every time you want to get diagnostics for a file under a different config.

Might want to look into how flow-mono-cli does it.

[thymikee]

Thanks for your input @rcjsuen @rvion, I'll definitely keep that in mind (@jbachhardie nice, wish this flow-mono-cli could be merged in some way to the official one)!

Since this extension is the official one, we decided to expose the LSP version behind a flag (flow.useLSP for now). This let us ship this PR earlier, work on missing features in, let's say, less blocking fashion and get beta-testers feedback without breaking the original (I call it legacy now).

[rvion]

@rcjsuen

1. it is not done in the language server. atom just spawn several lsp servers too, as expected for most mono-repo projects. This extension, as of now, does this, by finding .flowconfig and spawning servers.

2. vscode-flow-ls extension by @jbachhardie is a 60 lines long extension (https://github.com/jbachhardie/vscode-flow-ls/blob/master/lib/extension.js): it seems mostly copy-pasted from a basic lsp integration exemple, and it simply spawn a lsp server and add a footer item. I'm grateful to jbachhardie for making this, but I disagree with you when you say we should take his extension as a reference regarding what should be in this extension or not.

3. sure, good idea: https://github.com/rvion/flow-lsp-subfolders

Here: https://code.visualstudio.com/docs/extensions/example-language-server , you can see 3 links of lsp integration examples, 2 of them spawning multiple lsp servers: per root and per folder (what I ask to be kept). I made a quick and dirty extension some time ago based on this example spanning language servers in a for-loop with hardcoded path to my projects, it was working-ish, but I felt I was going to re-invent the wheel, and spend too much time creating a new extension while this one just work very well.

---

@jbachhardie

I'm pretty sure Flow itself doesn't support running multiple instances of its server simultaneously

No, Flow does support running multiple instances of its servers imultaneously

you're going to be paying a high performance cost of restarting

so no, absolutely no performance cost

---

@jbachhardie @thymikee

Might want to look into how flow-mono-cli does it.

I don't think so / I would advice AGAINST.
.flowconfig already support several flexible options related to monorepos like module.name_mapper or module.system.node.resolve_dirname. so far, I've found that supporting monorepos is just a 1 or 2 line config change in .flowconfig.

for this extension, all the pieces are alreay in the extension, I'm just asking about finding where .flowconfig are, and starting servers there.

also

• flow-mono-cli seems to imply yarn
• flow-mono-cli is a small project with few user (16 stars)

---

@thymikee

Sorry if I was not clear: this very extension (flowtype/flow-for-vscode) works perfectly as of now with mono-repos by starting several flow servers under the hood.

when I select a file in pj1 folder	when I select a file in pj2 folder

anyway, thank you very much for working on this plugin.

[rvion]

also,

facebook/flow@c5d21c6

flow lsp command landed yesterday in flow master
followed by several other flow-lsp related commits: https://github.com/facebook/flow/commits/master
from commit:

flow lsp will launch an "LSP adapter". The purpose of this LSP adapter is primarily just to manage the lifetime and connection status of the Flow server.

[rcjsuen]

Hi, @rvion. Thank you for getting back to me.

it is not done in the language server.

Sorry, could you clarify this? What is not being done in the language server? The handling of one directory with multiple subdirectories that each contain its own .flowconfig file?

vscode-flow-ls extension by @jbachhardie is a 60 lines long extension (https://github.com/jbachhardie/vscode-flow-ls/blob/master/lib/extension.js): it seems mostly copy-pasted from a basic lsp integration exemple, and it simply spawn a lsp server and add a footer item.

Yes, that is the point of that extension. To simply wrap the flow-language-server project as-is with as minimal of a configuration as possible.

I'm grateful to jbachhardie for making this, but I disagree with you when you say we should take his extension as a reference regarding what should be in this extension or not.

I was just trying to say that "if you install that extension and your use case works then the work that is currently progressing in this pull request should also satisfy your use case". That is not to say that if your use case isn't satisfied today then it won't be satisfied tomorrow.

Here: https://code.visualstudio.com/docs/extensions/example-language-server , you can see 3 links of lsp integration examples, 2 of them spawning multiple lsp servers: per root and per folder (what I ask to be kept).

Just to clarify, only 1 of the 3 examples spawn multiple language servers.

lsp-sample: Demos how to write a basic language server.
lsp-multi-root-sample: Demos how to write a language server which is multi workspace folder aware.
lsp-multi-server-sample: Demos how to write a language server that start a different server instance per workspace folder.

1. The first one just launches a language server without any bells and whistles.
2. The second one listens for configuration changes in each workspace root and sends them off to the language server. This is what they mean by "multi workspace folder aware". It knows about and will react to the concept of workspace folders but it only ever launches one language server.
3. The third one spawns a language server for each workspace folder. When you add/remove workspace folders to VS Code the extension will spawn/kill language servers accordingly.

On the topic of folders and workspaces, how do you start VS Code?

1. Do you launch VS Code onto a workspace file?

$ code stuff.code-workspace

2. Do you launch VS Code onto your project's folder?

$ code /path/to/flow-lsp-subfolders

3. Do you launch VS Code onto the parent folder of your project?

$ code /path/to/

[rvion]

@rcjsuen I won't respond more as you're starting to pollute this thread with non relevant questions, and sidetracking discussions.

I asked for an existing feature not to be removed. that's all.
the feature is: launch one flow-server per .flowconfig (or lsp compatible flow-server wrapper with this PR)

[jbachhardie]

No, Flow does support running multiple instances of its servers imultaneously

You are correct, I should've ran some tests before going on a fairly old issue in the Flow repo.

So yeah it's definitely possible and could technically be done at the extension level by managing multiple languageserver instances. We definitely should be managing at least one instance per workspace folder. I might give it a shot (and put it in a PR for this repo if it works).

[rattrayalex-stripe]

Better monorepo support would be fantastic. One thing to keep in mind is that not every subfolder with a .flowconfig uses the same flow version.

[vicapow]

Just to add color the discussion but a monorepo to be more useful the a collection of project folders would have a single flowconfig for all projects.

[thymikee]

@orta I think this is ready to review and merge. Based on that we can iterate on adding more features like coverage or improved diagnostics as followups

[orta]

Yep, changes are nice, small and atomic - I'm happy to give this a shot then.

[orta]

This is shipped as 0.8.0 - glad to see it moving 👍