Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
athanclark opened this issue Feb 16, 2019 · 11 comments
Closed

Comments

@athanclark
Copy link
Contributor

athanclark commented Feb 16, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

athanclark commented Feb 16, 2019

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
Copy link
Contributor

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
Copy link
Contributor

createLeafElement someContext.consumer { children: \value -> ... }

@athanclark
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Overall, I would much prefer to use #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
Copy link
Contributor Author

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
Copy link
Contributor

ethul commented Feb 17, 2019

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

athanclark added a commit to athanclark/purescript-react that referenced this issue Feb 17, 2019
@athanclark
Copy link
Contributor Author

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

ethul pushed a commit that referenced this issue Feb 22, 2019
* semicolons

* exposing `fragment`, fixing doc typos, adding docs in regards to #164

* better README

* typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants