Move blob/v*/lib/react.js from this repository to flow-typed

[rickyp-uber]

This is honestly the single most frustrating thing I've encountered as I move to flow. As a developer because the rest of my flow-type definitions exist in a flow-typed folder or in the node_modules folder it's easy to look up definitions. Because this isn't the case for react, it create a massive hurdle in terms of looking up specific react definitions.

The example problem scenario - I want to see the props related to a HTMLButton. In typescript you can simply go to definition of any element and you'll eventually get to ButtonHTMLAttributes. This works for other flow-type libraries as well, just not React.

Instead for flow typing for the react library, I have to have this url bookmarked in my browser to be able to look up dom attributes. Not the best developer experience in my opinion.

[EdwardDrapkin]

This is a Godsend: https://www.saltycrane.com/flow-type-cheat-sheet/latest/

[rickyp-uber]

It's depressing that the cheat sheet needs to exist in the first place but that will help me until this is resolved, thanks!

[rickyp-uber]

Just try finding the definition of SyntheticEvents for flow-types in Google... just try

[aikar]

You can download the file and place it in your flow typed folder. I did that for node

[rickyp-uber]

@aikar That's still a temporary workaround, it doesn't resolve the actual issue that this information should not live in this repository... or flow-typed should be merged into this repository.

[vicapow]

Any update on this? I understand priorities / feature discussions happen internally. Was there any discussion on this point someone internally can share?

[vicapow]

Hello, following up again. Could we have someone from the FB comment here?

[rickyp-uber]

I deleted this because I thought it was snarky but I'll put it back because it's funny
https://www.youtube.com/watch?time_continue=9&v=NAEppFUWLfc

[motiz88]

Disclaimer: Not at FB, not a Flow maintainer, not part of any behind-the-scenes discussions.

---

I think the main impediment to doing this is that lib/react.js is not the full picture. Some React-specific functionality is implemented in the OCaml code (e.g react_kit.ml), and there is no clean separation between the two (e.g. React.Element is a builtin).

In other words, as things stand, it's a matter of maintaining interdependent pieces of code in one place over moving one of them to another repo. Not that the dependency couldn't be refactored away in theory, but it's probably a big ask.

[rickyp-uber]

Thanks for the extra context, definitely did not know about the react_kit.ml portion. From an architecture standpoint it still seems like a good idea to separate the two so I'll leave this open but it is also good to know that the complexity level is high. Thanks!

[rickyp-uber]

Question for the maintainers, is this something you would be interested in investigating or possibly taking?

[vicapow]

cc: @avikchaudhuri

[vicapow]

cc: @gabelevi, @jbrown215, @samwgoldman, @nmote

Following up again. Would someone from the Flow core team be willing to provide some context? Is this something they'd be willing to accept a PR for?

[vicapow]

Following up again. Would appreciate any additional context the core team could offer on this.

[EdwardDrapkin]

Constantly bumping a ticket with a bunch of CCs on it is a great way to annoy the piss out of the maintainers and make sure this never happens. There's a huge backlog of issues and this is something that would require significant effort to make happen and offers only questionable benefit. Be patient and hope (or open a PR), but there's very little chance that this happens.

[nmote]

I'm not annoyed, I just don't have much to add. We've talked about moving all the libdefs into flow-typed but nobody is actively working on it and I don't know what complications there might be. I can say that I'd be surprised if we moved React over and not the other builtins.

[vicapow]

Thanks for the info, @nmote. Apologies if my pings are annoying. That was not my intent.

With that said, as it stands, it's still unclear if we did this work and submitted a PR if it would even be accepted. At the very least, would a PR be welcome?

[nmote]

I really don't know what would be involved to do this correctly. As others have mentioned, quite a bit of React logic is baked into Flow, so I doubt it would be as simple as just moving the file over. And if we do end up moving all of the libdefs over we would need to make sure that the libdef installation process is seamless and well-documented. Because of this, a PR that simply removed the React libdefs in favor of a flow-typed copy would probably not be accepted.

If somebody wanted to write up a project proposal that would be a good start, but I can't promise anything. The team is pretty busy so it might be hard to find time to properly review it.

[rickyp-uber]

Specifically targeting React as an example: Would moving it's flow definitions into a *.js.flow into the React npm module instead of flow-typed be more acceptable? What are the chances of React being okay with that addition (asking since I'm not inside Facebook 😄)?

[vicapow]

@rickyp-uber I would create a separate issue on the react github project so that the react folks will see it.

[rickyp-uber]

facebook/react#12553 (comment)

[rickyp-uber]

Just FYI, the react thread left off at a point I don't quite know how to refute.

The issue with this here is downstream React projects that also use Flow, e.g. React-Native. RN usually lags a few versions behind Flow (as do most projects that use Flow IME) and if you pin React to the latest version of Flow, then you prevent downstream React projects from updating to the latest React version until they update to the latest version of Flow. Some of the Flow releases require significant effort to upgrade because of changes (e.g. version 68 changed the way Flow interacted with unknown properties on objects and required a heavy refactor in some code). I don't think anything that makes upgrading React more difficult is a good idea unless the benefit that it presents is exceedingly obvious, and I'm still not sure what the actual benefit of moving the libdef would be.

The issue here (from what I get) isn't owning the types but it's more which version of flow-bin is required and having the type defined for that specific version of flow. This also is a curiousity as to how the *.js.flow files can define version compatibility ranges depending on what version of flow someone is running so if an older version of flow is running than what the definition defines, some error information or something occurs.

Basically, I think this is a great conversation to have, I'm just not the person to have it due to lack of knowledge of the architecture of flow and the direction it wants to go. Is flow-typed the main use case flow wants to support or was *.js.flow added to alleviate that requirement? Is there a decent counter argument of "Well why wouldn't you keep flow-bin up to date no matter what" coming into this debate?

[nmote]

I agree with that quote. I don't think this issue is specific to React -- I don't think that in general, it's a good idea to publish Flow types as part of a module, because then specific versions of that module can get pinned (inadvertently, through breaking changes to Flow) to specific versions of Flow. This is especially bad if your program uses multiple different libraries which are each pinned to a different Flow version.

The solution is to publish all libdefs to flow-typed. That way, you can have as many (Flow version, library version) pairs available as are required, and people who are not involved with the library are able to contribute library definitions.

The problem with this is that there is no way to take a library and automatically generate a libdef for it. This means that even if you already use Flow, you have to manually write your libdefs, which is a tedious and error-prone process. At some point we would like to provide such functionality, but we haven't been able to make it a priority yet.

In the meantime, we created *.js.flow files so that library authors can publish their original, type-annotated source along with their libraries. This is far from perfect, but it works okay in practice for many libraries.

Bottom line: long term we would like to move all libdefs to flow-typed (both those that are currently built in and those for npm packages) and then make Flow stop even looking in the node_modules folder in favor of the flow-typed definitions. It's going to be a bit of a project to do this, though, and we aren't sure when we will have the time.

Hope this answers your questions.

[rickyp-uber]

Yep thanks! Sounds like the goal is to get it into flow-typed not react lib as long term *.js.flow likely won't have the same support as flow-typed.

[EdwardDrapkin]

The problem with this is that there is no way to take a library and automatically generate a libdef for it. This means that even if you already use Flow, you have to manually write your libdefs, which is a tedious and error-prone process. At some point we would like to provide such functionality, but we haven't been able to make it a priority yet.

I have some experience doing this for local projects (e.g. we generate a flow type definition based on a configuration schema we feed to convict, so when we import config from 'our-config-package' Flow enforces the same schema that the config validator does). I don't, however, have much experience working in Ocaml (or any ML) for a decade now, so is there any reason this couldn't be done in JS and use flow.js to generate an AST to parse?

I started poking at flow-parser.js and I have one question: is it possible to merge dumpTypes into parse so that inferred types are available inside the AST, rather than just declared types? Would make this task much easier, otherwise the easiest way is probably going to be calling typeAtPos for identifiers that don't have type declarations as I walk the AST which I would guess is going to be significantly slower.

[SamChou19815]

Closing since some parts of the react libdefs are builtin in Flow. The declare module part can possibly be moved out, but we are generally able to keep this part up to date