facebook
Incorrect TypeScript Types
Many of the TypeScript definitions specify void as a type parameter for generic types where the equivalent Flow type definition uses an empty type parameter:
TypeScript Type Definition (🤨)
https://github.com/facebook/metro/blob/76413561abb3757285e0cb0305f1f9f616fa2b6c/packages/metro/types/IncrementalBundler.d.ts#L25
Flow Type Definition
https://github.com/facebook/metro/blob/76413561abb3757285e0cb0305f1f9f616fa2b6c/packages/metro/src/IncrementalBundler.js#L36
I would posit that the TypeScript definition is... mad sus, and I suspect we might just be running into this because the types are hand-written. Am I missing something and believing this is wrong? I'll open a PR to correct assuming this is indeed an incorrect typing.
I believe the TS types were originally generated by flow-api-translator plus some tweaking, and have been hand maintained since. They could well be wrong in places, but this might also be a translator bug.
CC @huntie
@nickhudkins Yup they're currently hand maintained, so would appreciate a targeted PR with test plan :)
@robhogan We should look at type translation for Metro again — there's been 15+ fixes to flow-api-translator this year alone.
Looks like flow-api-translator might be missing a test case for empty type parameters. I'll start there.
@robhogan heads up: https://github.com/facebook/hermes/pull/1676/checks. Looks like there might be a bit of setup for me to be able to run the jest tests that is not documented, in the shortest term would you mind allowing the workflow to run over there?
@huntie yep - but I mainly CCed to alert to the fact that if this is a bug in flow-api-translator it probably means the RN types are also broken.
@nickhudkins thanks for digging into this!
Running into quite a number of issues getting the js tests running:
This comment has me concerned if I will be able to get this running. Any further docs would be much appreciated, otherwise will continue to dig as to how to get this up and running:
https://github.com/facebook/hermes/blob/11c154bc365447648fa56fdd67a1c142034b85a6/tools/hermes-parser/js/scripts/build.sh#L24
Here's my story: https://github.com/facebook/hermes/discussions/1680
@nickhudkins Oh! Thanks for looking into this, but reckon it'd be quite hard to get flow-api-translator working completely for Metro right now (will require a bunch of codebase modernisation, alignments, Meta integration bits). Happy to support, but we may not have the time across the team to make this all happen that soon.
Do you have more info on the type OutputGraph issue so we can help unblock specifically?
Sure sure, so we use metro similarly to the way Expo uses it, though we forked it and have diverged quite a bit. I am spending some time trying to get aligned with upstream and do less custom things, and ran into this as we speak. Due to a myriad of unrelated things, I am unblocked, and just was doing my best to contribute back upstream.
Happy to contribute the changes manually for now with some light testing, otherwise I am also happy to keep chugging away at trying to resolve the flow-api-translator piece first if that's better aligned with the longer term approach.
In the meantime @huntie, a direct fix for any of the mismatched types: https://github.com/facebook/metro/pull/1608