React 16

[coot]

This solves #102. It may suffer from #105 if one is using props polymorphically, as in:

cls :: forall r. ReactRender r => ReactClass { r :: r }
cls = createClassStateless \{ r } -> r

it would be nice to have a warning in this case, but I think that's impossible. It would need to be shown only if one is passing cls to createElement directly but not if one is specialising the class. So it's ok to use the following goodCls:

goodCls :: ReactClass { r :: ReactElement }
goodCls = cls

but this leads to #105:

    -- somewhere in a render function
    render _  = pure $ createElement (cls :: ReactClass { r :: Array ReactElement }) { r: [] } []

[coot]

@paf31 Thermite is using a monoid structure on Spec that's based on array of react elements, something similar will be possible on the level of ReactClass-es. You can have a monoid which allows multiplication between ReactElement's or Array ReactElement by creating an Array whenever it's needed. That would make larger Thermite UI's more performant.

[paulyoung]

Are there any plans to merge this now that React 16 has been released?

[coot]

I would like to review this (against the release notes) before it's merged.

[coot]

Here are all the types that react supports:

• ReactElement
• String
• Number
• null
• Boolean
• portals - they need to be added to purescript-react-dom

Should we support null (and Boolean)?

I actually would prefer to not use a type class but a variant. This way one can render a list which consists of the various types, and avoids #105.

[coot]

Using a sum type will add a runtime overhead, since one will need to traverse an array and coerce its elements, so that's probably not the best approach. So at the end I think the class based approach is better.

[ethul]

I like the class-based approach. However, I would also be open to an implementation using a sum too.

On a side note, what does everything think about adding a class for createElement? I was thinking this might be a nice way to handle stateless classes instead of createClassStateless. For example, something like:

class ReactCreateElement a
instance reactClassReactCreateElement :: ReactCreateElement (ReactClass props)
instance reactStatelessReactCreateElement :: ReactCreateElement (props -> ReactElement)

createElement :: forall props. ReactCreateElement class => class -> props -> Array ReactElement -> ReactElement

[coot]

issue #118 @ethul this might be a bit surprising but it's best of what we can get.

[ethul]

Thanks for making these updates!

I think this is great to have the displayName set via the name of the function. However, I am wondering if we need to run it through the capitalize function. Would it be clear to the developer to simply just use the f.name as-is?

Also, on a side note, is the wW in hellowWorldCls intentional?

[coot]

It's just a convention in react ecosystem. Foreexample enzyme will look only for capitalised components.

The wW is just a typo.

[ethul]

Gotcha. Thanks for the info

[uzantnomo]

@coot it isn't merely a convention, lowercase and capitalized components are treated differently in JSX.

Changing displayName of a component might be confusing, and perhaps should be avoided, if it doesn't break any tools.

[coot]

That's a good point.

[coot]

@ethul what about merging this PR?

[ethul]

Thanks for the ping. I would like to take a bit more time to review all of these changes and dig into the display name further. I should be able to go through this over the next few days. Sorry for the delay!

[ethul]

References for displayName:
https://reactjs.org/docs/jsx-in-depth.html#html-tags-vs.-react-components
https://gist.github.com/sebmarkbage/f1f4ba40816e7d7848ad