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 type annotations #157

Closed
davidpfahler opened this issue Jun 14, 2015 · 13 comments

Comments

@davidpfahler
Copy link

commented Jun 14, 2015

As far as I can tell, flow type annotations are not supported. Would be an awesome feature.

@caspervonb

This comment has been minimized.

Copy link

commented Jun 14, 2015

Intended URL http://flowtype.org

@davidpfahler

This comment has been minimized.

Copy link
Author

commented Jun 15, 2015

@caspervonb thx, fixed it.

@nmn

This comment has been minimized.

Copy link

commented Jun 22, 2015

+1

@nmn

This comment has been minimized.

Copy link

commented Jun 22, 2015

@davidpfahler have you tried using Babel-eslint parser??

@nmn

This comment has been minimized.

Copy link

commented Jun 25, 2015

I tried to make it work in Atom but it didn't work. It worked in Sublime Text. I think this had something to do with it not picking up the custom parser from the package.json.

That said, there was still the problem of not having access to some of the types defined by flow like ReactElement this can worked around with the global trick.

@davidpfahler

This comment has been minimized.

Copy link
Author

commented Jun 26, 2015

@nmn would you mind posting your package.json configuration and the global trick you referred to?

@nmn

This comment has been minimized.

Copy link

commented Jun 26, 2015

@davidpfahler

The package.json has this in it:

"standard": {
    "parser": "babel-eslint"
  }

babel-eslint installed globally using npm i -g babel-eslint

The global trick is the one mentioned in the Readme:

standard doesn't know what any of the type names are. So for an annotation like ReactElement, I had to add this to the file

/* global ReactElement */

This got rid of the errors.

That said, for large files, this could quickly get out of hand as there will be tons of type annotations with different keywords and you'll have to declare all of them. Standard doesn't understand the common types such as number or string either.

I noticed that I was using the capitalised versions, which are incorrect of course.

@davidpfahler

This comment has been minimized.

Copy link
Author

commented Jun 28, 2015

@nmn Thank you very much. For the reasons you described above, I agree this is not useful, yet. standard would really have to understand all flow types.

@feross What would we need to do to implement flow support? I would love to create a PR, but I just have no idea where I'd need to start.

@feross

This comment has been minimized.

Copy link
Member

commented Jun 28, 2015

@davidpfahler We need to figure out what changes would be needed in standard. If there's something easy we can do to support Flow, I think we can consider it. Once the user has switched to babel-eslint, what errors do you still get?

Declaring all possible globals is out of the question, especially for framework-specific globals like ReactElement. That's a slippery slope. We don't even include any of the browser globals besides window and navigator because so many of them are poorly named, like name,length, etc. We opt instead to recommendwindow.location,window.name`, etc. Why is Flow declaring React-specific globals anyway? Seems like an odd decision.

It's interesting that babel-eslint is checking for number and string from the type declarations. The actual global class names are Number and String and these are built into standard.

Can you run standard --verbose and paste the errors you're getting?

@nmn

This comment has been minimized.

Copy link

commented Jun 28, 2015

@feross I've kind of got it working with babel-eslint. I would say the most important feature for flow/typescript support is to have some more config to list out globals. This would also help with web worker code, probably.

Both flow and typescript come with a long list of types. so if you can just define them all in one place that should work.

@hzoo

This comment has been minimized.

Copy link

commented Jul 1, 2015

Yeah before babel-eslint was just ignoring the types.

@feross babel-eslint checks for number since it's part of the parsing https://github.com/babel/babel/blob/2dfa6ddf36037506b35a66ef312d791897155d1c/src/babel/types/alias-keys.json#L81-L113

Yeah it's difficult since flow has it's own defined globals that ESLint won't know about - unless eslint can add a flow env or something for all those globals or you specify everything yourself.

The other solution would be for the user to import that type in every file import type ReactElement from './types'; or something yeah.

Related: babel/babel-eslint#130

@nmn

This comment has been minimized.

Copy link

commented Jul 1, 2015

@hzoo The number of globals introduced by Flow are not that many. When you're using just eslint, you can just add them in the config file. Mine looks like this:

"globals": {
    "ReactComponent": false,
    "ReactElement": false,
    "SyntheticEvent": false,
    "SyntheticClipboardEvent": false,
    "SyntheticCompositionEvent": false,
    "SyntheticInputEvent": false,
    "SyntheticUIEvent": false,
    "SyntheticFocusEvent": false,
    "SyntheticKeyboardEvent": false,
    "SyntheticMouseEvent": false,
    "SyntheticDragEvent": false,
    "SyntheticWheelEvent": false,
    "SyntheticTouchEvent": false
  }

If you set the environment to both browser and node, all the other globals are available anyway.
And the common primitive types like number and string seem to just work as well.

@feross

This comment has been minimized.

Copy link
Member

commented Jul 1, 2015

standard now supports declaring globals in package.json. So, if all that's needed to make Flow happy is to define a few globals, just paste them into package.json:

{
  "standard": {
    "globals": [
      "ReactComponent",
      "ReactElement",
      "SyntheticEvent",
      "SyntheticClipboardEvent",
      "SyntheticCompositionEvent",
      "SyntheticInputEvent",
      "SyntheticUIEvent",
      "SyntheticFocusEvent",
      "SyntheticKeyboardEvent",
      "SyntheticMouseEvent",
      "SyntheticDragEvent",
      "SyntheticWheelEvent",
      "SyntheticTouchEvent"
    ]
  }
}

I think this will work well enough for now :)

@feross feross closed this Jul 1, 2015

@taoeffect taoeffect referenced this issue Nov 30, 2016

@feross feross added enhancement and removed enhancement labels May 10, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants
You can’t perform that action at this time.