Skip to content
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

Investigate using purescript-react #29

Closed
alexmingoia opened this issue Apr 19, 2016 · 8 comments
Closed

Investigate using purescript-react #29

alexmingoia opened this issue Apr 19, 2016 · 8 comments

Comments

@alexmingoia
Copy link
Owner

alexmingoia commented Apr 19, 2016

@AppShipIt and @paf31 have asked why Pux isn't built ontop of purescript-react. The reason is that no functions from purescript-react are exposed in the Pux API. For example, Attribute a is used instead of Props, or Html a instead of ReactElement. When I wrote Pux, it was faster and less code to have Html a be a foreign data type corresponding directly to React.createElement.

Advantages

  • Share bug-fixes, elements, and attributes with the wider ecosystem.

Disadvantages

  • Additional types and function calls that aren't exposed in Pux's API.
  • Possible performance impact when rendering.

Performance

One concern is that if Html a translates to purescript-react's ReactElement, that adds an extra layer of function calls in the render method. The only place where performance is a real concern is the render method, because it blocks the application. Function calls have very little overhead but it's still a concern. Renders should be as lightweight as possible.

Thoughts?

@texastoland
Copy link

CC @spicydonuts

@megamaddu
Copy link
Contributor

megamaddu commented Apr 19, 2016

I'm not very familiar with purescript-react but it seems odd that both types wouldn't represent the same thing-- the createElement function <MyComponent /> compiles to in a normal React app. Is this issue with ReactElement affecting anything using purescript-react?

How does this relate to the helper functions for converting to/from regular React components? Would it make sense for Pux to continue using a custom implementation for simplicity/speed while also adding helpers for converting to and from ReactElements? (mostly wondering what's already been considered/discussed)

@dkoontz
Copy link

dkoontz commented Apr 20, 2016

Not sure if this is what you were asking about @spicydonuts, apologies if this is all redundant stuff you already know

https://github.com/alexmingoia/purescript-pux/blob/d921f696eb3896c18c7c695edbdfafee2e58b902/src/Pux.js#L8-L20&L41-L45

The Pux toElement creates a class that listens to the Signal HTML and forces a render when it changes as compared to the purescript-react createElement which mirrors React directly. Since Pux has moved to React + Elm Architecture it makes sense to me that Pux would not use React primitives directly and provide its own abstraction that it is free to swap around as needed to make the component level API as clean as possible (for example, by passing in the current Action & State to the update function). Using purescript-react under the hood if it makes things easier for development is a different matter entirely and one I am not qualified to speak on.

@texastoland
Copy link

I'm planning to dive in this weekend sorry for silence!

@alexmingoia
Copy link
Owner Author

I'm planning on releasing an update that changes Pux.toReact to return purescript-react's ReactClass props. This way people can use Pux components with purescript-react. I don't see any benefit to using purescript-react under the hood though.

@texastoland
Copy link

The TL;DR is you're delegating more to foreign JS and don't need its abstractions at the level you're interoperating?

@megamaddu
Copy link
Contributor

@alexmingoia Sounds great! It may still be worth continuing to explore building off of purescript-react internally, but that change at least makes it an internal-only issue.

@alexmingoia
Copy link
Owner Author

Pux v4.0.0 changes toReact to return ReactClass props from purescript-react.

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

No branches or pull requests

4 participants