purescript-contrib / purescript-react

React Bindings for PureScript
MIT License
400 stars 65 forks source link

Type of `children` prop field should dictate the value used in `createElement` #164

Closed athanclark closed 5 years ago

athanclark commented 5 years ago

Firstly, I think we should do-away with the Children foreign data type, which aims to represent the prop field. It's opaque, and doesn't accurately represent the purpose of the prop type.

The existence of the children prop, in React's perspective, means that the component should be treated as one that can have children (i.e. <Foo> ... </Foo>, rather than <Foo />).

I believe the createElement function should have a type similar to

createElement :: forall props children. ReactClass (children :: children | props) -> { | props } -> children -> ReactElement

(more or less, disregarding the ReactPropFields constraint). This would allow for natural semantic consistency with React 16.x's Context, where a Consumer has a children prop of children :: a -> ReactElement | ....

Furthermore, unitary components (i.e. <Foo />) would be created with a different function

createElement' :: forall props. Lacks "children" props => ReactClass props -> { | props } -> ReactElement

strictly because the ReactClass's props lacks the children attribute.

This way, the children attribute's value could just be coerced by its usage in createElement, whether it be an array of ReactElements, a single ReactElement, a fragment or whathaveyou. But more consistently, as a function for a Context's Consumer, making the paradigm more readily available for end-users.

natefaubion commented 5 years ago

The reason why Children is opaque is because in JSX it is not necessarily an array. The createElement binding is essentially (cls, props, children) => React.createElement(cls props, ...children). In JSX, <Foo></Foo> will yield null children, and <Foo>bar</Foo> will yield "bar" as the children (no Array wrapper). If I'm using a PS-created component from JS, then I'm probably not using the PS bindings to createElement and whatever magic we include, but instead it will likely generate direct calls to React.createElement. If you want parametric children, you can use createLeafElement. The difference is that createLeafElement does not accommodate children in any way for the purposes of a DSL, so you can have children be anything you want and it will pass it through unchanged.

athanclark commented 5 years ago

Okay, so, let me get this straight. Under-the-hood, children, as a prop field, is actually just an Iterable? or something thereof? That's a good justification for the opaque type, but I think we can do better.

In JSX, <Foo></Foo> will yield null children

Would that include <Foo />? Does this mean that <Foo /> is exactly equal to <Foo></Foo>? Isn't this technically a leaf node?

and <Foo>bar</Foo> will yield "bar" as the children (no Array wrapper).

Awesome, makes perfect sense.

If I'm using a PS-created component from JS, then I'm probably not using the PS bindings to createElement and whatever magic we include, but instead it will likely generate direct calls to React.createElement.

Okay, so from this use case, you're expecting to use a purescript-engineered React component from the regular React workflow in JavaScript? That makes good sense for why children would be considered opaque from purescript's perspective, because you'd never actually use it in purescript. However, I like building my entire application in purescript, and I'm sure a number a programmers would like to do the same. I think there could be a way to marry the two use cases for all end-users.

If you want parametric children, you can use createLeafElement. The difference is that createLeafElement does not accommodate children in any way for the purposes of a DSL, so you can have children be anything you want and it will pass it through unchanged.

Okay, here's where I'm lost. How would you just have "parametric children", when the type signature of createLeafElement includes no such usage of children at all? Like... isn't a leaf node explicitly one without children? Like <Foo></Foo> or <Foo />?

I am trying to bring proper type safety for the usage of children from within PureScript, using a PureScript engineered React component, inside another PureScript engineered React component. Again, my primary use case for this is a Consumer from the new Context paradigm, which explicitly types the children prop as a -> ReactElement, not the opaque Children, and furthermore is a function - such that it's end use case (from within purescript) would probably look like the following:

someContext :: Context Int

foo :: ReactClass (children :: Array ReactElement | props)

someElement :: ReactElement
someElement = createElement someContext.consumer {} \(x :: Int) ->
  createElement foo {...} [...]
natefaubion commented 5 years ago

createLeafElement is a binding to React.createElement(cls, props). So you absolutely can still have children as a prop and it's equivalent to <Foo children={..} />, which lots of linters actually warn about, but it means that the createElement API won't apply any magic when building the children from the spread args.

natefaubion commented 5 years ago
createLeafElement someContext.consumer { children: \value -> ... }
athanclark commented 5 years ago

I don't think this is how React was intended to be used though... from what I understand, the children prop should only be used from within the component itself, and from the "outside" you only interact with it via the 3rd argument as you alluded to above. Have you tested it to see if it works? And would you be willing to accept a patch / hard version change that more accurately represents the intended usage of it?

natefaubion commented 5 years ago

Yes, it works, and we use it quite frequently. children is just another prop, but the createElement API is designed to be used from JSX. It's API is the way it is because it's a trivial desugaring. createElement just takes the spread args, validates them, and assembles them as children.

natefaubion commented 5 years ago

And would you be willing to accept a patch / hard version change that more accurately represents the intended usage of it?

I guess that depends on what it is 😆 I don't agree that the current API does not reflect the intended usage, since the intended usage (JSX) does not apply to us. For me personally, I think it's important that we do not use anything other than a trivialcreateElement wrapper, because I need to be able to use components from JS. That is, I don't want to bake any special PS secret sauce into our own version of createElement.

natefaubion commented 5 years ago

Overall, I would much prefer to use https://github.com/purescript-contrib/purescript-react/issues/152, but I would need to use it first. I personally don't care about React's dev validation since I have a type system, but I would be concerned about adopting it in the library without observing that actual effects.

athanclark commented 5 years ago

Hmm okay, I'm sold :) sorry about the gripes, I just feel like the current API is just a bit unintuitive on how it should be used. How about some documentation patches?

ethul commented 5 years ago

Thanks for bringing this up. If you are interested in adding/updating documentation or examples in the README, a PR is certainly welcome.

athanclark commented 5 years ago

Awesome! I just submitted it. Thanks for your help @natefaubion!