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

Flow editing minor mode - updates & new features #1

Closed

Conversation

Fuco1
Copy link

@Fuco1 Fuco1 commented Oct 25, 2017

I'm opening this branch to track some updates to the mode (fixing incompatibilities which originated in some js2-mode changes and some additional features.

A review would be appreciated :)

Some issues:

  • In object literals, {foo: bar} the type is parsed even thought it is not a type. This breaks fontification and semantics of the object literals. I don't see a way yet to distinguish if the foo is a key or a variable.
  • The file name now conflicts with flow-minor-mode and causes infinite recursion when calling require. We need to rename the file to something else.
  • const valid: bool = ... fontifies bool as variable.
  • import type {Config} from 'types/Config' does not work.
  • Typing a destructured object does not work: function send(res, {body, content}: Response)
    • This breaks const a = foo ? {} : {}
  • Parse function<T> generic constructs.
    • Does not work with multiple generic arguments (separated by commas)
    • Does not work with typed generics such as function <T: number>
  • Support for sealed object types {| ... |}
  • dynamic object keys seem to be broken: return {[fieldName]: item.value} (error at :)
  • key type specification in objects should work: clients: { [client_id: string]: Client }

@Fuco1
Copy link
Author

Fuco1 commented Oct 27, 2017

This is getting quite ugly with all the state tracking variables. What I fear is that this will just spiral into us having to maintain our own state machine which will eventually blow up exponentially for all the combinations (we can't really use the method flow since we are using advices which are sort of outside of the normal recursion).

I'm not quite sure how to address this and if adding more and more flags is the way to go :/

@eqyiel
Copy link

eqyiel commented Nov 10, 2017

This is great, thanks for your work on this @Fuco1!

I've renamed flow-minor-mode.el to flow-js2-minor-mode.el locally in order to prevent the infinite recursion problem, is this something you'd be willing to do in your fork @antifuchs?

@Fuco1
Copy link
Author

Fuco1 commented Nov 10, 2017

@eqyiel There actually isn't any dependency on rjsx-mode so I'm wondering if it wouldn't be simpler to just put all this into a separate package.

@antifuchs any reasons I'm missing why you decided to extend this package?

In the vast majority of test cases, we want zero errors and
warnings. Then if there are any errors or warnings, it can help to see
what they are, so assert that in such a way that failing tests print
them.
This lets a user declare flow types on function arguments for now.
@Fuco1 Fuco1 force-pushed the flow-editing-minor-mode branch from f6a012a to c0441d6 Compare November 10, 2017 14:23
@antifuchs
Copy link
Owner

Oh wow - really nice work, everyone!! I'm sorry if you're running into constraints from my earlier decisions - most of the time it's that I didn't know what the consequences would be (and that I cargo-culted liberally from rjsx-mode directly).

If you want to pull this into a separate package, that'd probably be the cleanest solution. I currently have no time to work on this (have an intercontinental move to set up!), but would love if this keeps moving at the speed that you all have kept it at! (-:

@Fuco1
Copy link
Author

Fuco1 commented Nov 11, 2017

@antifuchs No worries mate! Just one thing, you're OK with me pulling your code into another package right? I think js2-mode cares about attribution so that if we would ever try to include it we should have things sorted out.

Good luck with your move, sounds exciting!:)

@antifuchs
Copy link
Owner

antifuchs commented Nov 11, 2017

Yes, that's absolutely OK - please take this code and organize it as you see fit!!

(Also, thanks for the kind wishes - I will need that luck!) (-:

@Fuco1
Copy link
Author

Fuco1 commented Nov 14, 2017

I've moved all the code along with the history to https://github.com/Fuco1/flow-js2-mode. Feel free to check it out and report issues there!

Thanks everyone for the support.

@Fuco1 Fuco1 closed this Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants