Skip to content
This repository has been archived by the owner on Nov 13, 2023. It is now read-only.

Support import of [@react.component] components whose props require conversion. #226

Merged
merged 13 commits into from
Aug 5, 2019

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Jul 29, 2019

Support import of [@react.component] components whose props require conversion.
The conversion is performed by wrapping the component with a React.createElement call,
so it works whether the component is a function or a class.

The wrapping conversion applies to all components in import position. For example, an imported component is in import position, but also a render prop of an exported component is.
No wrapping is performed for components in export position, relying on an invariant: if a component is in export position and requires conversion, then it is a function component (and not a class component). The invariant is maintained by construction for [@react.component] created on the Reason side, and by wrapping for components imported from JS (in import position).

All the TypeScript components are now typed with React.ComponentType<...>.
If existing code was using e.g. render props of type React.FC<...>,
direct function calls foo(props) should now be replaced with JSX calls <foo props=... />.

Support importing compoinents that require conversion while being defined as React classes in JS.
The function wrapping uses React.createElement to wrap the class component as a function component with the appropriate conversion for props.
This allows more flexibility in checking whether a function is an imported/exported component or not, where the type itself can be checked, not just the converter.
Import and export [@reaxct.component] components with type React.ComponenyType<...>.
Keep on using React.FV<...> for callbacks.
@cristianoc
Copy link
Collaborator Author

cristianoc commented Jul 31, 2019

@lalnuo @Coobaha this is relevant for issue #210.
Currently, to keep support for the use case in that issue, TypeScript components are imported/exported with type React.ComponentType<...>, however for render props such as renderMe, the type React.FC<...> is used.

This supports the uses case in https://gist.github.com/lalnuo/f8f3728476e9535115815dae97657eab, where renderMe is invoked from TS as renderMe({ randomString: 'foo' }).

However, if instead of a direct function call, JSX were used as in 5fde78b, then a cleaner solution would be possible, to always use type React.ComponentType<...> everywhere.

(Strictly speaking, the FC type would not be sound, because of the corner case where renderMe could actually be a class component imported from JS and then passed as prop).

How does that sound?

@cristianoc
Copy link
Collaborator Author

+@rickyvetter

…nd React.FC.

This drops the use of React.FC.
The ImportHooks example shows the changes required to user .tsx code in case it was using render props as functions directly. Now, one needs to use JSX calls instead. No change required if JSX was used already.
…Class.

Emitting ComponentClass when exporting via wrapReasonForJs is theoretically more precise, though it's not clear how to take advantage of the knowledge that a component is *not* a function components.

So it seems cleaner to always generate ComponentType instead.
Apply a wrap via React.createElement in all cases where a component requiring conversion is in an import position. E.g. when exporting a function components with one prop being a component type requiring conversion.

Add an example illustrating the case.
@cristianoc cristianoc changed the title Support importing class components with conversion. Support import of [@react.component] components whose props require conversion. Aug 5, 2019
@cristianoc cristianoc merged commit a7130b5 into master Aug 5, 2019
@cristianoc cristianoc deleted the import_class_component_with_conversion branch August 5, 2019 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant