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

Consider alternative ReactClass API #124

Closed
natefaubion opened this issue Nov 16, 2017 · 11 comments
Closed

Consider alternative ReactClass API #124

natefaubion opened this issue Nov 16, 2017 · 11 comments

Comments

@natefaubion
Copy link
Contributor

natefaubion commented Nov 16, 2017

We are using an alternative ReactClass API internally, and I think it should be considered for upstream.
https://gist.github.com/natefaubion/c639440f2fafd079c5d4eb36012d8e0e

It's basically the module pattern, but has several notable improvements:

  • It doesn't use create-react-class which is unsupported and has lots of unnecessary machinery
  • It's more flexible (we can allocate mutable cells, which gives us instance state, and obviates the need for custom ref handling and effects)
  • It's more efficient (we don't have to rebind event handlers on every render)
  • It forces you to use safe props (only records, no overwritten "key" or "children" props)
  • It uses Union for calculating the available lifecycle fields, so there's no need for special-casing componentDidCatch (only provide the ones you need).
  • It exposes PureComponent, and disallows shouldComponentUpdate in that case.
  • It's more amenable to abstraction since you get the ReactThis reference up front in the constructor, and you don't need to pass it around everywhere.
@natefaubion
Copy link
Contributor Author

natefaubion commented Nov 16, 2017

An example of how it fixes ref issues. Note this just uses purescript-refs and not a special ReactRef effect:

myClass = component "MyClass" \this -> do
  el <- newRef null

  let
    render = pure $
      D.div
        [ P.withRef (writeRef el) ]
        []
  
  pure { state: unit, render }

@natefaubion
Copy link
Contributor Author

natefaubion commented Nov 16, 2017

It should also be possible to recover some form a typeclass abstraction by monomorphizing ReactClasses within the constructor. It's a bit awkward, but possible.

@paf31
Copy link
Contributor

paf31 commented Nov 19, 2017

Sorry, what is the "module pattern"?

I'm all for discussing improvements to the API, but maybe we can break this down into some smaller individual changes?

Another thing I've been thinking is that there might be different sets of React bindings. The goal here was to have something low-level and minimal, although it's probably diverged a bit from that now. But there's no reason this couldn't be implemented as a separate library.

@natefaubion
Copy link
Contributor Author

The "module pattern" is just JS terminology for using a closure for private instance state and returning a record as the object's interface, rather than using new directly.

It's possible that this could be a different library, and we may do that, but I'd like to see if that's necessary. The core bindings here were written when createClass-style was the standard way of writing components, but the ecosystem has moved on from that (create-react-class is unsupported and the code can't even be found on GitHub anymore except in old tags). It's difficult to use the current API to do trivial things like ref tracking in an idiomatic way (key-based refs are deprecated). With the way ReactThis is threaded, event handlers must always be rebound on every render, which means we often render more than necessary due to shouldComponentUpdate failures. The API forces you to use component state for everything, which is not advised or encouraged for many things. I'm not saying that what we've written is necessarily the way forward, but I think it's probably the simplest and most straightforward way to write React components that tick all the boxes for current ecosystem expectations. I'm happy to go into more detail about the changes.

@born2defy
Copy link

I played with the Nate’s gist today and I really liked it. It allows for a much more idiomatic way of writing react components and is in better harmony with the way react is designed to be used. I feel like it’s a big step in the right direction. If we nix the effect types in 0.12, this will be a sweet react library. To me, the effects really get in the way. I’d rather spend the time actually fixing the bug they could have prevented then spend the time fixing the compiler errors. Especially when passing callbacks and wrapping components and all the things you want to do in react.

@natefaubion
Copy link
Contributor Author

FWIW I use EventHandler everywhere just so I don’t have to deal with effect rows in props. I don’t know why this type exists, and I had to write my own eliminator, but it’s an ergonomic win. ReactClass doesn’t preserve effect rows in its interface anyway so I don’t see why it should be preserved through event handlers.

@born2defy
Copy link

EventHandler and not EventHandlerContext? Could you show me how that works?

@natefaubion
Copy link
Contributor Author

@born2defy https://github.com/purescript-contrib/purescript-react/blob/master/src/React.purs#L439. EventHandler is just EffFn1. I assume this is for FFI purposes and predates EffFn. I just coerce it and give it the EventHandlerContext effect rows.

@paf31
Copy link
Contributor

paf31 commented Nov 26, 2017

I assume the idea would be to replace the spec type with the code you have there. Could you please make a PR so that we can see what it would look like?

@natefaubion
Copy link
Contributor Author

I've opened a PR here: #129

@ethul
Copy link
Contributor

ethul commented Mar 4, 2018

Closed by #129

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

4 participants