Alternative class construction #129

natefaubion · 2017-11-27T00:52:48Z

This removes createClass and spec as well as the dependency on
create-react-class in favor of component and pureComponent which
creates constructors that inherit from React.Component and
React.PureComponent in an idiomatic way for React. This also obviates
the need for some machinery like ref handling, which can be
implemented with purescript-refs instead.

This removes `createClass` and `spec` as well as the dependency on `create-react-class` in favor of `component` and `pureComponent` which creates constructors that inherit from `React.Component` and `React.PureComponent` in an idiomatic way for React. This also obviates the need for some machinery like `ref` handling, which can be implemented with `purescript-refs` instead.

natefaubion · 2017-11-27T00:55:07Z

src/React.purs

+  => RowLacks "key" props
+  => ReactRender render
+  => String
+  -> ReactClassConstructor (Record props) (Record state) render eff r


Note that I've required both props and state to be Records, as this is what React expects. I've removed the state boxing completely.

natefaubion · 2017-11-27T00:56:14Z

src/React.purs

-- | Then the `displayName` will be set up to `HelloWorldCls`
-foreign import createClassStateless :: forall props render.
-  ReactRender render =>
-  (props -> render) -> ReactClass props


I don't know why these functions exist since this can be implemented with PureScript functions. If we do want to add this back, then they can be implemented in PureScript on top of pureComponent.

natefaubion · 2017-11-27T00:56:41Z

src/React.purs

@@ -455,10 +378,6 @@ foreign import createElementTagName :: forall props.
 foreign import createElementTagNameDynamic :: forall props.
  TagName -> props -> Array ReactElement -> ReactElement

-- | Create a factory from a React class.
-foreign import createFactory :: forall props.
-  ReactClass props -> props -> ReactElement


createFactory is a deprecated API and the signature here is wrong anyway.

natefaubion · 2017-11-27T00:57:41Z

src/React/DOM/Props.purs

@@ -638,6 +620,10 @@ onWheel :: forall eff props state result.
  (Event -> EventHandlerContext eff props state result) -> Props
 onWheel f = unsafeMkProps "onWheel" (handle f)

+ref :: forall eff props state result.
+  (Nullable Foreign -> EventHandlerContext eff props state result) -> Props
+ref f = unsafeMkProps "ref" (handle f)


I've change ref to take a Foreign. Ref was a completely opaque data type, and the only way to validate it was by casting it to Foreign anyway.

The reason is that I would like to avoid a dependency on foreign which pulls in a lot of other heavy dependencies. I think it's desirable to keep this package as light as we can, since it's a reasonable use case to use PureScript for one-off React components, in which case DCE can be important.

If the user doesn't use ref functionality, won't the dependency be eliminated altogether in that case?

Well if they're using purs bundle, or if they're not using the React module as an entry point, then yes.

But I still don't see the reason to add a foreign dependency just for the Foreign type.

It's not a hill I'm gonna die on, I just think it's weird to export an API with an opaque type and provide no way for working with it :P.

Using toForeign isn't a big deal, but we should note it.

natefaubion · 2017-12-01T01:14:00Z

I've reverted the Foreign changes.

ethul · 2017-12-02T14:37:11Z

Thanks for working on this! I think the alternative class proposal is great. It is definitely nice to avoid create-react-class and use the component and pure component classes directly. Also, I think it's a good idea to use purescript-refs since it avoids issues like #126 as you pointed out.

I've tested out the changes a bit and have two comments.

First, I think it would be handy to continue to support React's functional components. If I understand it correctly, I think that functional components don't inherit from React.Component or React.PureComponent and don't have any lifecycle methods. I don't believe we would be able to implement them on top of pureComponent. But maybe there is a way to do this? I am open to discuss ideas.

If we decide to go with an implementation not using pureComponent, I wanted to propose a class-based solution. If this works for everyone, we could include it in this PR, or I am happy to open a new PR for discussion with an addition along the lines of:

class ReactCreateElement a

instance classReactCreateElement :: ReactCreateElement (ReactClass (Record props))

instance functionReactCreateElement :: ReactRender render => ReactCreateElement (Record props -> render)

createElement :: forall props a. ReactCreateElement a => a -> props -> Array ReactElement -> React.ReactElement
createElement = ...

Second, and this is a small point, but should the display name be moved into an optional property? I definitely see the utility in making it as a parameter to component and pureComponent, but I think it is technically optional.

Thanks again for working on this proposal!

natefaubion · 2017-12-02T16:43:37Z

First, I think it would be handy to continue to support React's functional components. If I understand it correctly, I think that functional components don't inherit from React.Component or React.PureComponent and don't have any lifecycle methods. I don't believe we would be able to implement them on top of pureComponent. But maybe there is a way to do this? I am open to discuss ideas.

It would just be component, not pureComponent. In PureScript, you can just simply write a function:

type Props = ...
myComponent :: Props -> Array ReactElement -> ReactElement
myComponent props children = D.div ...

Since we aren't using JSX, there's not really a reason to create a ReactClass from this. It can just be used directly. Reasons you might want a ReactClass:

Expose to JS consumers.
React will use the referential identity of the class to decide whether to rebuild a portion of the tree from scratch or diff it.

The above definition could be wrapped like so:

-- Omitting types because constraints
stateless name pureRender = component name \this ->
  pure { state: {}, render: pureRender <$> getProps this <*> getChildren this }

myComponentClass :: ReactClass Props
myComponentClass = stateless "MyComponent" myComponent

The main problem with overloading createElement via a type class is you still need to constrain the props (key and children). I don't think there's much to be gained in PureScript for overloading createElement in this way.

Second, and this is a small point, but should the display name be moved into an optional property? I definitely see the utility in making it as a parameter to component and pureComponent, but I think it is technically optional.

It's optional because React can infer the name from the name of the class definition in JS. "Classes" defined in PS however don't have a name since they are built from an anonymous function. You will want a name if you want to be able to debug your code using the traces React generates. If someone doesn't want to worry about names, it's easy enough for them to define:

hardToDebugComponent = component ""

natefaubion · 2017-12-02T17:35:26Z

An additional point against overloading createElement: because of the referential nature of react components/classes, I think overloading createElement would lead to anti-usages that have brittle performance. For example, it's really easy to want to do:

createElement (myFoo foo) props children

I think it makes more sense to opaquely lift a function to ReactClass than ReactElement as the expected usage is much more clear. I think it's better to think of the types as something like:

data ReactConstructor props state = ReactConstructor (ReactThis props state -> Eff ...)

data ReactClass props
  = LifecycleConstructor (Exists (ReactConstructor props))
  | PureRender (props -> Children -> ReactElement)

data ReactBoundComponent props = BoundProps (ReactClass props) props

data ReactElement
  = ReactElementComponent (Exists ReactBoundComponent)
  | ReactElementString String
  | ReactElementMulti (Array ReactElement)
  | ReactElementDOM ...
  | ...

born2defy · 2017-12-02T17:50:29Z

I agree with Nate. I am for simplifying the types and usage, rather than complicating them with additional classes. The closer to idiomatic React usage the better (imo).

ethul · 2017-12-02T21:05:27Z

@natefaubion Thanks for the information regarding the functional components. It definitely makes sense in terms of just writing a PureScript function and calling it directly from another react component.

However, one aspect I would like to better understand is how this works in JSX.

For example, given the following JSX:

var foo = 
  <div>
    <Bar name="a"></Bar>
  </div>
;

function Bar({ name }){
  return <div>{name}</div>;
}

Babel transpiles to:

"use strict";

var foo = React.createElement(
  "div",
  null,
  React.createElement(Bar, { name: "a" })
);

function Bar(_ref) {
  var name = _ref.name;

  return React.createElement(
    "div",
    null,
    name
  );
}

In the above, the function Bar is passed right to createElement. I am unclear what the advantages/disadvantages are of doing this versus just calling Bar({name: "a"}), and if we need/want to support this behaviour in this PureScript wrapper for React. I am open to going either direction, I just want to better understand what the implications, if any, are of omitting this capability from the library.

Pertaining to the type class overloading, I think your data-type proposal is a good one -- I am personally open to either approach.

I see that for the displayName, if it unset (i.e., made optional and not specified), the component's name comes up as Constructor. Just for discussion, it looks like it would be possible to let react infer the component's name if after

https://github.com/natefaubion/purescript-react/blob/90ff19d335966ad775970abb610612188001abc8/src/React.js#L35-L41

we added (and made displayName optional):

Object.defineProperty(Constructor, 'name', {
  value: ctrFn.name
});

Doing this will then allow react to pick up the name of the PureScript function that is passed to component; e.g.,

hello :: React.ReactClass { name :: String }
hello = React.component "Hello" hello'
  where
  hello' this = ...

would show hello$prime.

I don't know if this is a good or bad approach, but thought it might be worth mentioning.

natefaubion · 2017-12-03T00:47:03Z

I am unclear what the advantages/disadvantages are of doing this versus just calling Bar({name: "a"}), and if we need/want to support this behaviour in this PureScript wrapper for React.

React does not diff children of different component types. If I replace <Foo /> with <Bar />, React will destroy the entire tree under Foo, and rebuild Bar from scratch. If you call Foo(...) and Bar(..) directly, then there is no "component" to preserve and React just sees these as plain elements and will diff accordingly, which may or may not involved destroying the tree. This is unobservable if you are using functional components, but it is potentially advantageous for debugging with React dev tools.

We can definitely support this pattern "unboxed" like in React. My concern is that React attaches referential identity to these things, which is a semantic that PureScript function don't have. I think it's a better pattern in PureScript to explicitly lift these things to a ReactClass. ReactClass is opaque and we've attached no semantics to it, so there's no reason for it to be an instance of Component and it can encapsulate both stateful and functional components. I think it's somewhat obvious that it's bad to create classes inline; it's not obvious that you should not apply arguments to functions inline (due to pervasive referential identity).

Pertaining to the type class overloading, I think your data-type proposal is a good one -- I am personally open to either approach.

FWIW, I'm not suggesting that we should use explicit PS ADTs to model anything, just that it's a useful way to think about how React elements work.

Doing this will then allow react to pick up the name of the PureScript function that is passed to component; e.g.,

This is a personal opinion, but I don't care for this kind of magic in a PS API. I understand not everyone feels this way. In JS, stateful components will always have a display name, since you must define a class with one. I think this is something you really want with stateful components, since those are the things that are actually going to go wrong and you might want to debug. I think it's reasonable to not care in the case of functional components, since it's already common for functional components to not have display names, and functional components don't go wrong.

ethul · 2017-12-05T02:12:43Z

Thanks for the rundown @natefaubion. Very helpful. Good point about React attaching a referential identity to the functional components. Given this, I agree with you that perhaps ReactClass is more appropriate for these functional components. I suppose if we have a compelling use-case for supporting functional components like the way React does, then maybe it would be worthwhile adding in a function to lift them to a ReactClass as you mentioned. We can always add-in support if/when an explicit need arises too.

Having an explicit displayName is probably safest. In #109, it came up that a library expected a certain naming convention in order to recognize components. Having the component author provide the name definitely works.

natefaubion · 2017-12-14T19:13:59Z

Something else I'm trying out, internally (after removing the RowLacks constraints):

createElement
  :: forall required given optional children
   . Union given optional (key :: String | required)
  => ReactClass (Record (children :: children | required))
  -> Record given
  -> children
  -> ReactElement
createElement = createElementImpl

This allows for typed children, and for an optional "key" prop.

natefaubion · 2017-12-14T19:15:32Z

One problem with this is the polymorphism-in-record problem we always hit.

jgoux · 2017-12-20T18:02:47Z

I'm not sure if it was discussed before, but could we take inspiration from https://github.com/reasonml/reason-react ?

natefaubion · 2017-12-20T18:11:10Z

As far as I can tell, it's basically untyped:
https://github.com/reasonml/reason-react/blob/master/src/ReasonReact.rei#L42

jgoux · 2018-01-22T20:59:50Z

I'm really interested in this PR. Is there something blocking it?

born2defy · 2018-01-22T21:23:51Z

For what it's worth, I have been using this formulation for a few weeks and it works quite nicely.

ethul · 2018-01-27T14:55:50Z

I think these changes look good. Thank you for all of the work on this.

@natefaubion is there anything that you are trying out that you would like to include in this PR?

@paf31 just checking in to see if you had any suggestions regarding this PR.

Thanks!

natefaubion · 2018-01-27T16:47:04Z

@ethul I haven't had a chance to use the above createElement alternative in anger, but I think it's interesting. One issue with adding things like RowLacks to the class construction is that you can't have row polymorphism in your props (anything requiring polymorphism has to be boxed under a single prop). In that sense I quite like deferring the constraint to createElement. It does mean that props must always declare what children they expect, but this is useful because it means you can just transparently get props.children without materializing an additional array.

natefaubion · 2018-02-08T16:37:25Z

After looking into it some more, I don't think that transparently proxying the children works because it isn't parametric over arrays. React does some ad hoc magic to collapse empty children to null, and to remove the wrapper for single children. I'm open to suggestions about the RowLacks issue and polymorphic props, if it's even an issue in general.

Edit: To expand, you'd have to do implicit boxing to keep it parametric like we've done in the past with state, and I don't want to do that since it makes interop weird.

ethul · 2018-02-24T16:29:22Z

@natefaubion Agreed that deferring the constraints to createElement looks to be a nice way to go.

Regarding children, would make sense to have it as a Children foreign data type? Or maybe this is not ideal?

natefaubion · 2018-02-24T17:37:09Z

@ethul Yes, I think one option is:

createElement
  :: forall required given optional children
   . Union given optional (key :: String | required)
  => ReactClass (Record (children :: Children children | required))
  -> Record given
  -> Array children
  -> ReactElement
createElement = createElementImpl

createLeafElement
  :: forall required given optional children
   . Union given optional (key :: String | required)
  => ReactClass (Record required)
  -> Record given
  -> ReactElement
createLeafElement = createLeafElementImpl

For reference: https://flow.org/en/docs/react/children/

ethul · 2018-02-24T17:54:10Z

@natefaubion Looks like that option could work out nicely. Thanks for the reference and code snippets.

Are you willing to incorporate these latest ideas into this PR?

natefaubion · 2018-02-24T18:52:30Z

@ethul Sure, I'll give it a try.

ethul · 2018-02-24T18:54:38Z

Great! Thanks

…

On Sat, Feb 24, 2018 at 13:52 Nathan Faubion ***@***.***> wrote: @ethul <https://github.com/ethul> Sure, I'll give it a try. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYy98LVUrCZn723OC9V5J3mbN7iDF4ks5tYFpvgaJpZM4QrACz> .

natefaubion · 2018-02-24T21:08:42Z

The Children API in React is not parametric either, and iterating over them only yields nodes. If we wanted to support typed nodes, then we'd have to do something crazy like having a custom kind that we'd use to index each ReactClass and ReactElement. I don't think it's worth it, personally, so I think we should just keep an opaque, monomorphic Children in the above signatures.

createElement :: forall required given optional.
  Union given optional (key :: String | required) =>
  ReactClass { children :: Children | required } ->
  Record given ->
  Array ReactElement ->
  ReactElement
createElement = createElementImpl

createLeafElement :: forall required given optional.
  Union given optional (key :: String | required) =>
  ReactClass { | required } ->
  Record given ->
  ReactElement
createLeafElement = createLeafElementImpl

You can still provide other typed children parametrically using createLeafElement and manually supplying the children prop.

testChildren :: ReactClass { label :: String, children :: Children }
testChildren = component "TestChildren" \this -> do
  let
    render = do
      props <- getProps this
      pure $ div'
        [ text props.label
        , div' (childrenToArray props.children)
        ]
  pure { state: {}, render }

testTypedChildren :: ReactClass { label :: String, children :: Int -> ReactElement }
testTypedChildren = component "TypedTestChildren" \this -> do
  let
    render = do
      props <- getProps this
      pure $ div'
        [ text props.label
        , div' [ props.children 42 ]
        ]
  pure { state: {}, render }

test :: ReactElement
test = div'
  [ createElement testChildren { label: "Hello", key: "test" }
      [ div' [ text "World" ]
      , div' [ text "Children" ]
      ]
  , createLeafElement testTypedChildren
      { label: "Typed"
      , children: \i -> text (show i)
      }
  ]

natefaubion · 2018-02-25T01:26:15Z

So playing with this some more, what you really need to make it type safe is:

class ReactPropFields (required :: # Type) (given :: # Type)

instance reactPropFields ::
  ( Union given optional (key :: String | required)
  , Union optional leftover (key :: String)
  ) =>
  ReactPropFields required given

createElement :: forall required given.
  ReactPropFields required given =>
  ReactClass { children :: Children | required } ->
  { | given } ->
  Array ReactElement ->
  ReactElement
createElement = createElementImpl

But this again prohibits you from having polymorphic props.

Edit: It doesn't prevent you from having polymorphic props, but you do need to add a type annotation.

testPolyProps :: forall r. ReactClass { label :: String | r }
testPolyProps = component "PolyProps" \this -> do
  let
    render = do
      props <- getProps this
      pure $ div'
        [ text props.label
        ]
  pure { state: {}, render }

createLeafElement (testPolyProps :: ReactClass { label :: String, foo :: Int })
  { label: "Hello"
  , foo: 42
  }

ethul · 2018-02-25T13:10:48Z

Thanks for making these changes. I think it works to have a monomorphic, opaque, Children type. The class looks interesting however. I will dig into this update and do some testing. Thanks again!

…

On Sat, Feb 24, 2018 at 20:26 Nathan Faubion ***@***.***> wrote: So playing with this some more, what you really need to make it type safe is: class ReactPropFields (required :: # Type) (given :: # Type) instance reactPropFields :: ( Union given optional (key :: String | required) , Union optional leftover (key :: String) ) => ReactPropFields required given createElement :: forall required given. ReactPropFields required given => ReactClass { children :: Children | required } -> { | given } -> Array ReactElement -> ReactElement createElement = createElementImpl But this again prohibits you from having polymorphic props. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYy_u9DqKSm-4h6Vmtuuq_BqOVzxa6ks5tYLa4gaJpZM4QrACz> .

natefaubion · 2018-02-25T17:11:57Z

@ethul This is probably a little more clear, but generates an extra constraint argument.

class ReactPropFields (required :: # Type) (given :: # Type)

instance reactPropFields ::
  ( Union given optional all
  , Union optional leftover (key :: String)
  , Union required (key :: String) all
  ) =>
  ReactPropFields required given

This just makes the key prop optional, and prevents you specifying it in the props for your component.

ethul · 2018-03-03T17:20:52Z

I apologize for the delay. I think these changes look great. I had a chance to write a few components with these updates and the module is quite nice to use. The update to ReactPropFields looks good if you are interested in making the change. Thanks again for your work on this. Please just give me a heads up confirming this PR is good to merge.

natefaubion · 2018-03-03T17:53:36Z

Sorry, I should have clarified. The updated ReactPropFields is equivalent, just phrased with an extra Union constraint. I should probably update it with a ref callback as well.

natefaubion · 2018-03-03T18:29:16Z

@ethul How do you feel about addressing #131? I think this PR is good to go, but I think we should address it before making another breaking release.

ethul · 2018-03-04T00:28:51Z

Thanks for making these changes. I think it makes sense to address #131. If you’re game to include it into this PR, please feel free. We can also do a separate PR before making the next major release if that makes more sense. I also wonder if there are any other breaking changes we should bring in before doing the next major release.

…

On Sat, Mar 3, 2018 at 13:29 Nathan Faubion ***@***.***> wrote: @ethul <https://github.com/ethul> How do you feel about addressing #131 <#131>? I think this PR is good to go, but I think we should address it before making another breaking release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYywUevi_sfbzkFlGiT8B7bstDNfOxks5tauD8gaJpZM4QrACz> .

natefaubion · 2018-03-04T02:55:44Z

I think I'd like to merge this PR as is and make a new one for the render changes.

ethul · 2018-03-04T03:15:21Z

Sounds good.

…

On Sat, Mar 3, 2018 at 21:55 Nathan Faubion ***@***.***> wrote: I think I'd like to merge this PR as is and make a new one for the render changes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYy5MxEQ9pRLBCZrkHYfFfphrMmq6cks5ta1exgaJpZM4QrACz> .

ethul · 2018-03-04T12:26:28Z

Thanks again!

natefaubion commented Nov 27, 2017

View reviewed changes

natefaubion mentioned this pull request Nov 27, 2017

Consider alternative ReactClass API #124

Closed

Revert Ref -> Foreign

90ff19d

Move prop constraints to createElement

18adc4b

natefaubion added 2 commits March 3, 2018 10:10

Add ref as a reserved prop

c7c39cc

Add statelessComponent

d7233a0

ethul merged commit c03129a into purescript-contrib:master Mar 4, 2018

This was referenced Mar 4, 2018

createFactory is unnecessary #128

Closed

Restrict props to Record #119

Closed

error when pattern matching on props #104

Closed

Add support for using EventListener values in a ReactSpec with access to ReactThis #60

Closed

paulyoung mentioned this pull request Apr 13, 2018

Remove createFactory? #142

Closed

Alternative class construction #129

Alternative class construction #129

Conversation

natefaubion commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natefaubion commented Dec 1, 2017

ethul commented Dec 2, 2017

natefaubion commented Dec 2, 2017

natefaubion commented Dec 2, 2017 • edited

born2defy commented Dec 2, 2017

ethul commented Dec 2, 2017

natefaubion commented Dec 3, 2017

ethul commented Dec 5, 2017

natefaubion commented Dec 14, 2017

natefaubion commented Dec 14, 2017

jgoux commented Dec 20, 2017

natefaubion commented Dec 20, 2017

jgoux commented Jan 22, 2018

born2defy commented Jan 22, 2018

ethul commented Jan 27, 2018

natefaubion commented Jan 27, 2018

natefaubion commented Feb 8, 2018 • edited

ethul commented Feb 24, 2018

natefaubion commented Feb 24, 2018

ethul commented Feb 24, 2018

natefaubion commented Feb 24, 2018

ethul commented Feb 24, 2018 via email

natefaubion commented Feb 24, 2018 • edited

natefaubion commented Feb 25, 2018 • edited

ethul commented Feb 25, 2018 via email

natefaubion commented Feb 25, 2018

ethul commented Mar 3, 2018

natefaubion commented Mar 3, 2018

natefaubion commented Mar 3, 2018

ethul commented Mar 4, 2018 via email

natefaubion commented Mar 4, 2018

ethul commented Mar 4, 2018 via email

ethul commented Mar 4, 2018

natefaubion commented Dec 2, 2017 •

edited

natefaubion commented Feb 8, 2018 •

edited

natefaubion commented Feb 24, 2018 •

edited

natefaubion commented Feb 25, 2018 •

edited