-
Notifications
You must be signed in to change notification settings - Fork 111
Conversation
changes useLSP mode - use `flow lsp` to implement language server (remove flow-language-server) - add TypeCoverage - add Status - add Rename - add `stopFlowOnExit` support - add `runOnEdit` partial support (only report syntax errors)
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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I've tried to run this locally, but can't see flow status or an option to enable coverage (tried settings as well), also looks like the hover middleware is not kicking in, as I see text as plaintext when hovering. Is there any other special setup I need to make to test it properly? EDIT: tested on Flow 0.81 and 0.87 |
@thymikee did you compile the code? |
yup, with cmd+b (got confirmation that everything was compiled by Babel) |
Remove your old build folder and run compile again. May be old flowLSP.js file present in build dir. |
LOL, that worked 🤦♂️ |
: ) We have to fix the compile script. |
Done |
Works really well, thank you so much for taking that over! I could never find time to do it properly. I guess that cc @orta give it a spin |
Is there any error log in plugin console? For me rename works fine. Make sure you save files after renaming. |
None in the extension console, but I get this output in the Panel's Output tab:
Anyway, rename doesn't work for me in most cases and it's not really reproducible, seems to be happening randomly, regardless window being restarted etc. |
Which version of flow are you using? As this bug was fixed in flow 0.84 (see issue). |
Oh yea, I forgot I switched to the other project with older Flow to test it as well. I got this bug on 0.81, and taking a look now, I couldn't see it in 0.87 so you're right about the fix. And rename seems to now work on 0.87 as expected now (I have no idea why it wasn't first time I tested it on this project). |
Currently the hover output doesnt look very good (using js-beautify). I will send a separate PR replacing it with prettier. |
I was about to suggest that. |
Locally I am using prettier only but not included in this PR to keep this PR small. If it's fine then I will push the commit. |
How do you feel about removing the duplication in status bar? One label should be just enough What I think makes sense is to put the coverage message the same as the status when restarting server, and make it e.g.
Changes in |
OK, cool, I'll give this a spin today 👍 |
other changes - implement status actions
This is feeling awesome! Let's get this in ASAP. IMO, feels good enough to warrant LSP being the default. |
To get flow to pass, I think you'll need to add these bits of the vscode dts to the flow-typed definition in the repo, should remove most errors |
changes - add type defn for vscode-* packages - fix flow errors - update bundled flow to v0.75 NOTE: this can be breaking change for users using plugin bundled flow (v0.68) - update flow-typed defn
changes - request coverage only if connected to server - recompute coverage when server gets connected (will fix missing coverage for initial file)
FWIW, flow officially recommended vscode + this plugin as the recommended way to use Flow: https://twitter.com/flowtype/status/1073258441723006981 It would be really awesome if LSP becomes the default configuration! |
.flowconfig
Outdated
|
||
[version] | ||
^0.68.0 | ||
^0.75.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most recent flow version is 0.89.0. Is there a reason you don't want to use the most recent version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason for updating to 0.75 is to avoid breaking changes to users relying on bundled flow 0.68. I was planning to use v0.89 in a separate commit. Anyways, we released v1.0 so it's fine.
|
||
dispose() { | ||
this.statusBarItem.dispose(); | ||
this._clearTimeout(); | ||
this._command.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we dispose StatusProvider
as well, just not to forget about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee Actually there is nothing in StatusProvider
to dispose so I kept it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember a comment about adding such in StatusProvider. Maybe worth removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I should remove the onStatus
listener in dispose method. As provider and status component both disposes together it will not cause any issues but I will push a fix thanks.
OK, I've added a changelog entry, bumped this package to version 1.0 because this feels like the right time, and addressed the rest of the feedback in this PR |
I'm gonna merge, ship and build this, and not announce for a day or two to see if any issues rain in. It's been working fine for me in the Jest codebase, which has been a pretty good indicator for me in the past. |
Congrats @Mayank1791989 and thanks! |
@orta and @Mayank1791989: Would one of you want to write a blog post about flow-for-vscode 1.0 and the features it supports? We could put it on the flow blog and tweet it out! |
@jbrown215 that sounds like a good idea, @Mayank1791989 can you email me to say hi (my email is in my GH profile) ? |
Fix most of the issues listed in #243.
Changes
stopFlowOnExit
supportrunOnEdit
partial support (only report syntax errors)flow lsp
(remove flow-language-server).flowconfig
syntax highlighting