fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
273 stars 67 forks source link

add ComponentType<'Props> to the binding. #173

Closed matthid closed 4 years ago

matthid commented 4 years ago

run npm audit fix, and add .ionide to gitignore

Please review.

alfonsogarciacaro commented 4 years ago

Hi @matthid! Can you please provide an example of when this is useful? With Fable.React 5 I was trying to make the bindings easier to use by reducing the number of possibilities. If there are too many interfaces to represent a component, users may get confused (me included). How is this different from ReactElementType<'props> for example?

matthid commented 4 years ago

Can you please provide an example of when this is useful?

I used this type to properly implement typings for react-table where you can input elements with specific props types as properties. There is no Type where this can be expressed. In particular ReactElementType<'props> is not implemented by FunctionComponent or Component so using it will just make everything unsafe (ie you need to case or unbox)

How is this different from ReactElementType<'props> for example?

Apparently ElementType includes JSX elements (string) in this case, while ComponentType doesn't:

https://flow.org/en/docs/react/types/#toc-react-elementtype

matthid commented 4 years ago

Generally, I think we should try to mirror the typescript names as closely as possible otherwise we need our own docs. Also we should have all types defined, even when we decide they should be aliases. Otherwise we also create confusion for people trying to get code from ts2fable to compile

alfonsogarciacaro commented 4 years ago

Sorry, but you have to think on the types/interfaces as they appear here not as they are used in Typescript or Flow. Fable.React.ReactElementType doesn't include string in this library:

https://github.com/fable-compiler/fable-react/blob/fa43005e82490945e2a59beedf86054ae1e929a3/src/Fable.React.fs#L9-L13

You have helpers to convert functions, type declarations or html tags in a "type-safe" way (even if we're just casting under the hood):

https://github.com/fable-compiler/fable-react/blob/fa43005e82490945e2a59beedf86054ae1e929a3/src/Fable.React.Helpers.fs#L74-L98

FunctionComponent is just an alias of 'Props -> ReactElement. It doesn't implement ReactElementType because it's not an actual React component (I agree the name is misleading), it's just a function that calls React createElement (and a couple of other tricks) with the given props. This is because we are not using a preprocessor like JSX so we have to call createElement on our own.

In the latest version we chose to be a bit more opinionated and not follow Typescript declarations in the believe this would help users. Maybe this was not the best choice but I'm sure no one wants more breaking changes :) Of course, nobody is forced to use this library when interacting with React. Other alternatives can be provided as they're appearing for example for React DSL.

matthid commented 4 years ago

Thanks @alfonsogarciacaro. So you are trying to tell me that I could just use ReactElementType<'props> in the binding and use the of* functions in the ReactElementType module.

So maybe there is no difference then.

In the latest version we chose to be a bit more opinionated and not follow Typescript declarations in the believe this would help users.

I think this approach is fine but we need to have some documentation on which typescript types correspond to what (like some table). In the end I think both are true:

Maybe we can add a type alias for ReactElementType with the proper name? I don't think that would be a breaking change.