Support for flow types

[danez]

I started working on adding support for flow types.

So far I only tried to get it working for classes, and it already works nicely for this usecase, but only for string type and declared types. The code although is far away from nice yet. It also needs a patched version of ast-types, which I will create a PR also soon.

I opened this PR already so that interested people can join and already have a look and maybe give feedback.

Will fix #31

This needs to be done:

• support es6 classes
• support createClass()
• support stateless components()
• support all types from flow
  • primitive types
  • class types
  • object signature
  • literal types
  • function signature
  • callable-object/function-object signature
  • maybe types
  • union
  • intersection
• support inline type definitions
• support declared type definitions
• extract description from type
• handle imported type definitions (not really sure if we can do anything about that)
• add tests
• update ast-types to support Identifiers to TypeAlias
• make code clean :)
• adapt Readme

[facebook-github-bot]

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 - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[fkling]

Nice, thank you! TBH, I had something different in mind with flow type support. I thought we could run flow directly and query it to get type information (though I don't know how much information we can get).

However, if we can do it this way and get equally good results, that's great!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[danez]

I tried flow itself before, but the ast it delivers is nearly the same as the one from babylon. The only current drawback with babylon is that imported/required types or types which are defined as libs for flow, cannot be resolved. But I also don't know how one would do that with flow. That might be a future addition if someone finds a way to do that.

I'm nearly done with all the codechanges. I probably might refactor smaller pieces, but I think there wont be large changes anymore.

I will now start updating the readme and add a section about flow, and what are the differences to propTypes. As flow offers more granular types, the results (except primitive types) are a little bit different.

[fkling]

I tried flow itself before, but the ast it delivers is nearly the same as the one from babylon.

I was thinking that flow can return us the type information already parsed, but since I haven't worked with it yet, I might be wrong.

Either way, I am really happy about this :) When you are done, I'd appreciate it if you could squash everything into a single commit.

[danez]

Yes squashing everything was the plan. :)

[danez]

Do you think that the flowHandler should be part of the default handlers?

[phpnode]

This is fantastic, very much looking forward to using it. Is there anything I can help with to get this landed sooner?

[danez]

Ok I am done from my side, as far as I can see.

The only remaining question is if the flowTypeHandler should be part of the default handlers (currently it is)? This probably depends on how you want to release this feature: With this enabled by default I would think it is more a 3.0 version (as it could generate different output than before for people), whereas if it is not enabled by default, then it could be 2.7. But thats up to you @fkling .

If you want me to change things pls tell me.

[fkling]

Mmh, not sure, what would you prefer? Would it make sense to have a --flow option on the CLI which will use flow handlers, instead of standard ones, if they exist.

[danez]

First of all flow- and propType-handlers don't conflict with eachother. Having both active is not a problem. The only "problem" that could arise is if people use both propTypes and flow in their components. In that case one of the handlers (depending on the order) would overwrite the other one.

But even if they don't conflict, I like the idea of having a --flow option in the cli, which adds the flow handler to the defaultHandlers if set and disables the propType handlers.

For the api people can build their desired set of handlers anyway on their own.

If thats all okay, I add that?

[fkling]

The only "problem" that could arise is if people use both propTypes and flow in their components.

Right. I think (hope?) that this is not very common. What's more interesting is how often people use flow type annotation and propTypes in the same code base (in different components)(which would be the case for Facebook). Those might want to have both handlers enabled.

However, I think it would fine to have this option first (no major update needed) and then see if others need a more flexible setup.

[danez]

Okay I added the cli option.

[fkling]

Actually, here might be a better idea:

Lets simply add the flow prop types to a new key, e.g. flowProps, and add the handlers to the defaults. This way there won't be any conflicts and both annotations can be used (for static and runtime checks).

How does that sound?

[danez]

But then you end up having defaultValues under props and the types + description under flowProps which to me is a little bit counterintuitive.

I was thinking about something similar: Having the flow type under flowType inside the props object, next to type. The only question then is what to do about the description, maybe flowDescription although that sounds confusing to me.

[fkling]

Having the flow type under flowType inside the props object, next to type.

Seems fine too.

The only question then is what to do about the description, maybe flowDescription although that sounds confusing to me.

Do you think people would add a description to both, the propTypes and the flow types in the same component? I think it's fine to assume that only one of them will be annotated. We could make it so that either the flow type description handler or the propType description handler doesn't set a description if there already is one set.

[danez]

That seems a reasonable approach. React-docgen could also warn if there are multiple descriptions found.

[danez]

Ok i changed the flowType from type to flowType.

And both docgen handlers respect each other and don't update the description if already set, but trigger a warning.

I removed the cli option again and added flowTypeHandler and flowTypeDocHandler to defaultHandlers.

[fkling]

Two comments inside (somehow I'm always adding the comments so that GH doesn't show them inline, but whatever).

If that's clarified I think we are good to go! Really awesome work, I'm sure lots of people will be happy about this, I certainly am :) Thank you!

[danez]

You were right about the type<->flowType thing in the readme, I did not regenerate it with the latest changes. Done now.

[danez]

And yes you are right, the required field is also set by the flowTypeHandler, although I think this is less critical than the type itself i would say.

In case someone really uses both propTypes and flow, the type can be completely different, but the required field should be the same, And I think it would count as a bug in the components if you specify a prop as required with propTypes but not with flow or the other way around.
Do you think it is worth adding a check if the other handler already added the required field?

[fkling]

Do you think it is worth adding a check if the other handler already added the required field?

I think its fine for now. If we encounter any problems, we can always update later. Everything looks really great! I'm only a little concerned regarding the warnings, since there is no way to disable them. This might be surprising if react-docgen is used as a module instead of the CLI.

OTOH, I think it would be valuable if there was some information about unresolved types. But I also don't want to block this on implementing a proper error handling feature.

Should we just omit the warnings for now?

[danez]

Thats true. There is also a note in the readme about this unresolvable types.

[danez]

Okay I removed them now.

[fkling]

Thank you! I'm going to merge this now, and release a new version later today.

Also thank you for your patience for my many questions and requests, and again, for implementing this feature!

[danez]

@fkling If there are bugs coming in, feel free to ping me.

[benjamn]

Looking forward to that ast-types PR :)