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

Encoding optional props with ReactPropFields #151

Closed
jvliwanag opened this issue Jun 5, 2018 · 11 comments
Closed

Encoding optional props with ReactPropFields #151

jvliwanag opened this issue Jun 5, 2018 · 11 comments

Comments

@jvliwanag
Copy link

On the current master, createLeafElement has the signature:

-- | Create an element from a React class that does not require children.
createLeafElement :: forall required given.
  ReactPropFields required given =>
  ReactClass { | required } ->
  { | given } ->
  ReactElement

I used to be able to use Union for encoding react classes with optional prop fields defined such as:

foreign import fooClass :: forall props. ReactClass props

foo :: forall o t
           . Union o t ( title :: String )
           => {|o}
           -> ReactElement
foo = createLeafElement fooClass

Wherein I can call foo {} or foo {title: "hello"}. But it seems the new ReactPropFields constraint now prevents me from doing this. Any suggested way getting around this?

@ethul
Copy link
Contributor

ethul commented Jun 5, 2018

Out of curiosity, which version or commit was this working on?

@natefaubion any thoughts?

@jvliwanag
Copy link
Author

Union approach works properly v5.1.0 with:

foreign import createElement :: forall props.
  ReactClass props -> props -> Array ReactElement -> ReactElement

I'm currently trying to upgrade doolse/purescript-reactnative which uses the Union approach for optional props.

@natefaubion
Copy link
Contributor

natefaubion commented Jun 5, 2018

The type of ReactPropFields and createElement is determined by the type of the ReactClass provided to it, however you have forall props. ReactClass props. This type would need to be instantiated to something. There is no way to safely express what you want with createElement because there's no way to verify that forall props. adheres to Reacts special cases around props, since by definition we don't know what it is.

@jvliwanag
Copy link
Author

If that's the case, how should I define fooClass and foo if I want both of the ff to work?

  • foo { title: "hello" }
  • foo {}

@natefaubion
Copy link
Contributor

PureScript doesn’t have optional fields, so what you’d do is use Record.merge with some set of default props before passing it in. Otherwise I would import fooClass as ReactClass {} instead and use unsafeCoerce in your smart constructor.

@jvliwanag
Copy link
Author

Hm, I guess I've been wanting Option 1 as discussed in #147. This has worked nicely in purescript-reactnative with purescript-react 5.1.0.

My problem with the default props is that the defaults are not always known esp for foreign react components. I'll probably just try out unsafeCoerce for now.

Alternatively, why not have two row types for ReactClass? One for required as it stands now, and another for optional. Then the getProps for entries in optional could be wrapped in Maybe. The createElement should then accept a single record composed having all required rows and possibly optional rows.

jvliwanag added a commit to jvliwanag/purescript-reactnative that referenced this issue Jun 5, 2018
* Have to use createElement with unsafeCoerce due to
  purescript-contrib/purescript-react#151
* Change createNoChild to use createLeafElement
@natefaubion
Copy link
Contributor

My problem with the default props is that the defaults are not always known esp for foreign react components.

What do you mean by this? Do you mean something like pass-through props? You can construct a ReactClass with polymorphic props, but you must instantiate it to a monomorphic type when using it based on the props you are giving it.

Alternatively, why not have two row types for ReactClass? One for required as it stands now, and another for optional. Then the getProps for entries in optional could be wrapped in Maybe. The createElement should then accept a single record composed having all required rows and possibly optional rows.

This is an orthogonal issue to what the constraints are trying to accomplish. Right now it's impossible to use createElement to construct something with a key or ref. That's because key and ref are not part of the ReactClass specification, but rather part of the interface to createElement, yet React requires you to mix these things together in the same bag of props, which is very unfortunate since it is so awkward to type. I'm happy to consider keeping the old signature, and exporting this as an alternative. I've also opened #152 to provide an alternative which does not use createElement at all and is easier for us to type.

@jvliwanag
Copy link
Author

Default props - when integrating third party components, with lots of optional props (say https://facebook.github.io/react-native/docs/view.html#props), it is:

1 - tedious to find out which default each prop should take
2 - sometimes (1) is not really practical since the default is most likely null/undefined
3 - (2) would then end up with ugly type signatures (full of Nullable).

As such a Union based API is just much more friendly to work with. I understand that ReactPropFields is used just to introduce key and ref props. The issue is that it prevents from a Union based API to work properly. That said #152 does seem to provide a nice alternative.

--

If we want to embrace optional fields though, and we're open to change the ReactClass definition, why not have explicit sections for required and optional props? Meaning:

foreign import data ReactClass :: # Type -> # Type -> Type

button :: ReactClass (label :: String, onPress :: Int) (border :: Int)
button = ...

mkButtonElement1 =
  createElement button { label: "Hello"
                       , onPress: 5
                       , border: 5
                       }

mkButtonRequiredOnly =
  createElement button { label: "Hello"
                       , onPress: 5
                       }

See gist for full example.

Imo, this definition would mirror how most JS defined react components are used. When working with purescript react components, we can maybe change getProps to emit a record containing all required props and wrapping all optional props with Maybe.

I can work on this if this is an acceptable approach.

@ethul
Copy link
Contributor

ethul commented Jun 6, 2018

Regarding integrating with third-party components, I think I prefer the implementation mentioned in Option 2 - Builder in #147, albeit more time consuming to write. The main reason is that I think it is handy for defining props where you might want a more complex data type for the prop value. In the React Native View example, if you wanted data PointerEvents = Auto | None | BoxNone | BoxOnly, mapping that to a string could be done in the builder function for the pointerEvents prop. I do think Option 1 is more convenient, but I think Option 2 is more flexible with respect to data types.

@natefaubion
Copy link
Contributor

Imo, this definition would mirror how most JS defined react components are used. When working with purescript react components, we can maybe change getProps to emit a record containing all required props and wrapping all optional props with Maybe.

I don't know how to do this without significant overhead. A single type for props reflects the actual React interface. As far as implementation goes defaultProps is part of the createElement interface, and in reality all it does is merge prop records, which is exactly what I recommended above. Trying to hook into getProps make everything much more complicated and error prone. There is no reason you can't define a separate ReactDefaultClass as you've stated with an appropriate constructor to ReactElement, and it would still absolutely be compatible with this library.

@jvliwanag
Copy link
Author

I'll continue to experiment approaches for this, but for now, the unsafe variants on createElement that's part of #149 should do. Thanks!

jvliwanag added a commit to jvliwanag/purescript-reactnative that referenced this issue Jun 10, 2018
* Have to use createElement with unsafeCoerce due to
  purescript-contrib/purescript-react#151
* Change createNoChild to use createLeafElement
jvliwanag added a commit to jvliwanag/purescript-reactnative that referenced this issue Jun 10, 2018
* Have to use createElementUnsafe, createLeafElementUnsafe due to
  purescript-contrib/purescript-react#151
* Change createNoChild to use createLeafElementUnsafe
* Change from Eff eff to Effect
* Fix imports
* Change unsafe components to require record props
doolse pushed a commit to doolse/purescript-reactnative that referenced this issue Jun 12, 2018
* Upgrade bower deps for 0.12

* Upgrade to purescript-react 6.0.0
* Replace purescript-eff with purescript-effect
* Replace purescript-maps with purescript-foreign-object
* Add purescript-avar dev dependency

* Upgrade to purescript 0.12, purescript-react 6.0.0

* Have to use createElementUnsafe, createLeafElementUnsafe due to
  purescript-contrib/purescript-react#151
* Change createNoChild to use createLeafElementUnsafe
* Change from Eff eff to Effect
* Fix imports
* Change unsafe components to require record props

* Upgrade test to 0.12

* Upgrade travis, package.json, package-lock.json to 0.12
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

3 participants