paf31 / purescript-thermite

A simple PureScript wrapper for React
MIT License
350 stars 55 forks source link

Use of div' in render causes problems with react-native #67

Closed doolse closed 7 years ago

doolse commented 8 years ago

Because there is no "div" component in react-native you get an error when using thermite. I made a workaround which only uses a div if there is more than one element in the array but it would be better to make it customisable. I believe the equivalent in react-native would be to use the "view" component. Here is my work around for reference:

render :: React.Render props state eff
render this = map maybeDiv $
  spec.render (dispatcher this)
    <$> React.getProps this
    <*> React.readState this
    <*> React.getChildren this
    where
      maybeDiv elems = fromMaybe (div' elems) if (length elems == 1) then head elems else Nothing
paf31 commented 8 years ago

Interesting, thanks for the report. I'll have a think how we can improve this.

It could be as simple as requiring a function argument of type Array ReactElement -> ReactElement.

sudhirvkumar commented 7 years ago

@doolse Thanks for the code... its a very useful idea.

pkamenarsky commented 7 years ago

@doolse Are you using thermite with react-native in production?

doolse commented 7 years ago

@pkamenarsky I'm actually using my own library on top of halogen - https://github.com/doolse/purescript-halogen-reactnative but it's by no means production ready yet. I've just been adding components as I need them thus far.

sudhirvkumar commented 7 years ago

@pkamenarsky @doolse We are using thermite for react native applications we are still in the development phase and API will have breaking changes.

You can check it out in https://github.com/sudhirvkumar/purescript-rnx/tree/thermite.

This will be merged with into https://github.com/atomicits/purescript-rnx and released as purescript-rnx package

We are working on a client project with this library. We appreciate feedback :)

doolse commented 7 years ago

@sudhirvkumar Just browsing through your repo it looks like there's been a lot of work done for adding proper purescript types for everything, really nice work! Can I suggest that you keep the react-native properties and classes as a separate library so that it's not tied to thermite.

With https://github.com/doolse/purescript-halogen-react I've made my own React element tree abstraction but it's dawned on me that I shouldn't have bothered with an extra layer of abstraction, it just adds unneeded complication. In fact using Halogen itself is probably overkill when your rendering to react, as React already does most of what Halogen is doing.

The only reason I chose to use Halogen over Thermite was that it looked more mature and seemed easier to deal with child components, however I've found that combining components "the React way" with a ReactClass and event handler properties for communication is much easier than using Lenses to handle child state and actions. Not to mention that I don't think performance would be good on a react native client with only ReactClass if it has to redraw the entire tree for every frame.

So I've gone off topic a bit there but I think I'd like to use & contribute to your library, I already have a couple of codebases which are using purescript-halogen-react (1 react native, 1 web) and if I start converting those over I will have a lot of feedback for you :)

sudhirvkumar commented 7 years ago

@doolse can we continue our conversation here? https://github.com/atomicits/purescript-rnx/issues/23

pkamenarsky commented 7 years ago

@sudhirvkumar Please make an announcement when your bindings become mature enough. Many people are interested in an alternative to ClojureScript for react-native development, and purescript-thermite seems to be the perfect fit.

sudhirvkumar commented 7 years ago

@pkamenarsky yes, we are working towards stabilizing the API as we speak. We are working on a client project with the RNX library. So we will also have a production app with the library, so it will be battle tested too when we release it.

sudhirvkumar commented 7 years ago

@paf31

It could be as simple as requiring a function argument of type Array ReactElement -> ReactElement.

If that can be done then we wont need a wrapper div and also we will be able to use thermite directly instead of forking it to make changes for React Native Development.

https://github.com/atomicits/purescript-rnx/blob/master/src/RNX.purs

The only change is the view instead of div. Appreciate your comments.

Thanks in Advance!

paf31 commented 7 years ago

Sure, would you mind making a PR? I think it can be done in a non-breaking way, right?

sudhirvkumar commented 7 years ago

@paf31 only non breaking and quick way I can think of is @doolse 's code

render :: React.Render props state eff
render this = map maybeDiv $
  spec.render (dispatcher this)
    <$> React.getProps this
    <*> React.readState this
    <*> React.getChildren this
    where
      maybeDiv elems = fromMaybe (div' elems) if (length elems == 1) then head elems else Nothing

This works, and we need to be careful about the render function implementation in React Native as it should have only one element in the Array. I can live with that for now!

If we have to change the signature of the Render then its a breaking change and also have to figure out how to create Monoid and Semigroup instances. Right now as the Render function returns an Array ReactElement its easy to create Semigroup and Monoid instances for the Spec.

I will create a PR with doolse's code.

doolse commented 7 years ago

I've made a pull request which makes it a function passed in to createrReactSpec' instead, which I think is what @paf31 had in mind.

sudhirvkumar commented 7 years ago

Thanks @doolse @paf31