Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #7119 - Add support for Facebook Flow language #7208

Closed
wants to merge 4 commits into from

Conversation

jdelStrother
Copy link
Contributor

@jdelStrother jdelStrother commented Sep 28, 2016


👉 There is a bounty source opened on this issue, click on the badge for more info

Bountysource


Feature list in order to complete this PR and unlcok the bounty:

Auto-completion

  • The layer should activate company-flow whenever the buffer begins with either of these lines:
    • // @flow
    • /* @flow */

[Author note: It's currently enabled in all of react-mode and js2-mode, and I've not noticed any ill-effects in files where flow isn't present.]

Syntax Highlighting

  • The layer should improve syntax highlighting for flow. Right now it's mostly ok in react-mode and quite bad in javascript-mode (for instance, use of generics Map<K, V> breaks the syntax highlighting). There may be some inspiration in emacs-flow-jsx, react-mode and typescript-mode.

Documentation

  • The type of current word should be rendered with eldoc - flow type-at-pos should give the type.
  • bonus points for highlighted eldoc like in typescript.

Code Navigation

  • Using flow get-def and flow find-module should enable amazing jump to definition functionality. I'd love to have them binded to g or d or at least local leader.

Original message

Hi there - I've cobbled this together from the various flow-related things I've been using. There seems to be some interest in having a proper spacemacs flow layer (#7119), so thought I'd throw this out for discussion.

There's a few bits that might be somewhat targeted towards my own setup - for example, I chain the flow flychecker onto the eslint checker, but conceivably others might want to chain it onto jshint instead. How much of that would you want to see customizable before merging this?


EDIT: [@syl20bnr] changed title and message body to reflect the bounty source attached to it.

@pheuter
Copy link

pheuter commented Oct 6, 2016

this is great, thanks so much for creating a layer, I use flow every day and now I can switch back to Emacs for my web development 😄

Fwiw, I was only able to get the proper company-flow completions to show up when I disabled tern-mode:

(remove-hook 'react-mode-hook 'tern-mode)

Otherwise, it was only populating completions from tern instead of flow.

@jdelStrother
Copy link
Contributor Author

@pheuter ah, I actually have tern disabled globally via dotspacemacs-excluded-packages.

If people are using tern, do you think they'd prefer to have flow completions or tern completions? I'm not sure how good tern completions usually are. (I'm also not sure how I'd disable tern based on the presence of the @flow directive, so if anyone has any suggestions...)

@jdelStrother
Copy link
Contributor Author

Discovered that eldoc-documentation-function would potentially hang emacs for several seconds if you made a change to a file that required flow to restart its server. I've changed the flow invocation there to flow --retry-if-init=false and rebased onto the latest develop.

@evindor
Copy link

evindor commented Jan 24, 2017

I've started a bounty for this feature in #7119 Bountysource

@aaronjensen pointed out that the "whole thing has suffered from feature creep, and it's unclear what the bounty is for".

The bounty is for improving, and then merging, this pull request. Which effectively closes #7119.

I suggest the following list to consider the bounty requirements satisfied:

  • (autocompletion) The layer should activate company-flow company-mode whenever the buffer begins with // @flow or /* @flow */
  • (syntax) The layer should improve syntax highlight for flow. Right now it's mostly ok in react-mode and quite bad in javascript-mode. (for me use of generics Map<K, V> breaks the syntax after this line) There may be some inspiration in emacs-flow-jsx, react-mode and typescript-mode.
  • (eldoc) The type of current word should be rendered with eldoc, bonus points for highlighted eldoc like in typescript. flow type-at-pos should give the type
  • (jump-to-definition) Using flow get-def and flow find-module should enable amazing jump to definition functionality. I'd love to have them binded to g d or at least local leader.

With these it would become very comfortable working with flow in spacemacs. There are a bunch of interesting extras like flow find-refs and flow coverage.

@aaronjensen
Copy link
Contributor

(autocompletion) The layer should activate company-flow company-mode whenever the buffer begins with // @flow or /* @flow */

This would require work in company-flow because right now it is enabled on a major mode basis. If there was a major mode for flow, then we could just use that, but it would need to be derived from something (I'd prefer rjsx-mode, but others may prefer js2-mode), so it's complicated. Maybe flow-mode should be a minor mode? I wouldn't be sure how to only enable that in buffers w/ the @flow attr. Also, basing it on @flow will break for people that have [options] all=true in their .flowconfig. I'd probably prefer to just look for .flowconfig.

@syl20bnr syl20bnr changed the title Initial stab at a flow-type layer Add support for Facebook Flow language Jan 26, 2017
@syl20bnr syl20bnr changed the title Add support for Facebook Flow language Fixes #7119 - Add support for Facebook Flow language Jan 26, 2017
@jdelStrother
Copy link
Contributor Author

@evindor do you have a fuller example of highlighting not working with generics? They seem ok to me in js2-mode.

@evindor
Copy link

evindor commented Jan 29, 2017

@jdelStrother I've run a few experiments and looks like reduced the problem to a few edge cases:

Argument type annotations

No Flow - all good:
screen shot 2017-01-29 at 21 05 50

Add some typing:
screen shot 2017-01-29 at 21 06 37
Notice how arguments to both functions lose the highlight

Generic in the argument type annotation:

export type Handler<T> = {
  [key: string]: (state: T, action: Action<any>) => T,
}                                       // ^^^ This is the troublemaker

All good:
screen shot 2017-01-29 at 21 10 56

Let's break it:
screen shot 2017-01-29 at 21 12 42
@jdelStrother look how almost all highlight after Handler<T> declaration is gone.

Fixable by extracting this Action<any> to the outside:
screen shot 2017-01-29 at 21 16 51

@evindor
Copy link

evindor commented Jan 29, 2017

I don't think these are the only cases, I'll get back to work and if I see anything else I'll report here. Also, in general, can we have some kind of highlight for the annotations themselves?

Here I rewrote this sample piece to typescript:
screen shot 2017-01-29 at 21 29 28

@evindor
Copy link

evindor commented Jan 29, 2017

Regarding eldoc, check out this typescript glory:

screen shot 2017-01-29 at 21 34 31screen shot 2017-01-29 at 21 35 10

@jdelStrother
Copy link
Contributor Author

jdelStrother commented Jan 29, 2017 via email

@evindor
Copy link

evindor commented Jan 29, 2017

@jdelStrother well, these are bells and whistles, can live w/o fancy colored eldoc.

@evindor
Copy link

evindor commented Jan 29, 2017

@jdelStrother for the same piece of code, tried react-mode:
screen shot 2017-01-29 at 21 50 09

Look like the highlight isn't screwed up anywhere! I've looked into react-mode and they don't define any extra syntax, only derive from web-mode.

Then I switched to web-mode and the code was still looking like on the screenshot above (taken in react-mode)

@evindor
Copy link

evindor commented Jan 29, 2017

Please excuse me for linking here cause it's not very readable, but I guess there should be some inspiration available here in web-mode

@jdelStrother
Copy link
Contributor Author

jdelStrother commented Jan 30, 2017

I think fixing up js2-mode's highlighting for flow is probably beyond me - their parser is pretty terrifying. See mooz/js2-mode#224 for the relevant issue.

However, I did manage to get highlighting working in eldoc using web-mode's highlighter - will push a fix for that later.

@jdelStrother jdelStrother force-pushed the flow-type branch 2 times, most recently from 2e4f84c to ad6d50e Compare January 30, 2017 18:32
@z3t0
Copy link
Contributor

z3t0 commented May 16, 2017

How do I add this to my spacemacs setup?

@orther
Copy link

orther commented May 16, 2017

@z3t0 I personally just replaced my 2 year spacemacs confit with @aaronjensen 's setup and I'm loving life. Yeah a bit lazy and some of my custom stuff got bumped but for now it's a great way to go. Besides that there are several examples above. Would be happy to help.

@orther
Copy link

orther commented May 16, 2017

PS as a bonus w/ prettier auto formatting. That pretty much life changing and integrate with flow.

@z3t0
Copy link
Contributor

z3t0 commented May 16, 2017

@orther I'm not seeing the setup, could you share a link? thanks

@orther
Copy link

orther commented May 16, 2017

This is his repo I'm referring to. It also fixed my Elixir formatting. Not to meantion the rjsx mode for react and JS is better imo @z3t0

@orther
Copy link

orther commented May 16, 2017

@z3t0
Copy link
Contributor

z3t0 commented May 16, 2017

got it thanks

@codeincontext
Copy link

codeincontext commented May 21, 2017

Just set up flow-type-minor and it's working well with aaronjensen's rjsx mode. I do get a small lag for about a second just after opening a file. I can live with it for now though.

My config's at https://github.com/skattyadz/dotfiles/tree/master/.spacemacs.d/layers in case you're interested in doing something similar

@z3t0
Copy link
Contributor

z3t0 commented May 28, 2017

@skattyadz Thank you for the link but I am getting the following error

Sorry it's an image file because I couldn't figure out how to keep the error there long enough to copy it.

@DrPandemic
Copy link

I would like to add my grain of salt here. I have been playing with this PR for maybe one week. Before that I was using my mode which is mostly copy pasted code from you and aaronjensen. Your mode is pretty good for my use cases, but I would like to point out some of my pain points. Note: most of my testing was made with react-mode and all my issues are related to react-mode.

Company

By default with js2-mode, company-flow is present in company-backends. But when I switch to react-mode the flow backend is not there anymore. Earlier in this PR, you talked with @pheuter about tern-mode vs company-flow. IMHO, if flow is working company-flow is highly superior to tern-mode. It gives better situational content, less useless completion and documentation.

Major

I played with js2-mode, rjsx-mode and react-mode and I think that react-mode has the best syntax coloring and is the most stable. I have some issues with react-mode, but less than with the others. So my question is, should this mode set a default major mode for flow/javascript files? If it does, I would argue that react-mode should be chosen.

Eldoc

Eldoc is not present with reac-mode.

@Fuco1
Copy link

Fuco1 commented Nov 13, 2017

FWIW I've started some work (we're tracking it antifuchs/rjsx-mode#1) to extend js2-mode parsing to support flow... so this will provide you with complete AST as well as all the goodies of js2-mode. It's mostly usable for the 90% use-case now. In the coming days I will turn it into a separate package as there is no relation to rjsx (react).

It will provide syntax highlighting as well just like js2-mode does.

@syl20bnr
Copy link
Owner

Any news about this bounty ?

@Fuco1 created a repository there: https://github.com/Fuco1/flow-js2-mode

@Fuco1
Copy link

Fuco1 commented Jun 16, 2018

I don't know how to integrate it with spacemacs nor will I try to do that anytime soon but if anyone wants to use the package I've created feel free to do so.

@smile13241324 smile13241324 self-assigned this Dec 8, 2019
@smile13241324
Copy link
Collaborator

@jdelStrother I have merged the first draft version of your layer provided #10513. I think this should be working as a first draft however to get full ide features I would recommend adding a proper LSP server for this.

Anyway this merge made this PR obsolete.

@jdelStrother
Copy link
Contributor Author

@jdelStrother I have merged the first draft version of your layer provided #10513. I think this should be working as a first draft however to get full ide features I would recommend adding a proper LSP server for this.

Yeah, I've been experimenting with that recently - possibly we should just delete this layer entirely and suggest that people use:

dotspacemacs-configuration-layers '(
  (javascript :variables
      javascript-backend 'lsp))

?

@wukkuan
Copy link
Contributor

wukkuan commented Dec 27, 2019

@jdelStrother - I think that sounds like a much better solution, FWIW.

@smile13241324
Copy link
Collaborator

@jdelStrother @wukkuan ok so this is included in the current lsp implementation, I haven't been sure about this, then it would suffice to add some hints to the javascript layer's documentation and let users use lsp if they require flow support. Care to make a PR?

@wukkuan
Copy link
Contributor

wukkuan commented Dec 29, 2019

@smile13241324 - I'll take a shot at figuring out how to set this up and then documenting it.

@smile13241324
Copy link
Collaborator

@wukkuan - thanks for doing the PR :), and sorry for my late answer I am currently pretty busy at work so there is not as much time for spacemacs as I would like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.