Consider alternative ReactClass API

[natefaubion]

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]

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]

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]

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]

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]

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]

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]

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

[natefaubion]

@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]

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]

I've opened a PR here: #129

[ethul]

Closed by #129