Closed elliotdavies closed 5 years ago
Thanks for looking into the Ref
functionality. If you're interested in creating a PR with just the createRef
API separately, I think that would be great! Feel free to use anything from #159 if that helps.
After looking into this over the last couple of days, I'm struggling to find a pleasant implementation that supports both the new createRef
API as well as the old callback style. Below is a problem summary, a few of the approaches I've tried, and then some thoughts on how to proceed. It'd be useful to get some feedback before I make a call on it.
There are two ways to create refs (createRef
and callbacks) and two places that refs can be passed (DOM elements and React classes), so we have four cases. In pseudo-Purescript:
myClass :: React.ReactClass {}
myClass = React.component "MyClass" \this -> do
domRef <- Ref.createDOMRef
instanceRef <- Ref.createInstanceRef
pure
{ state: { domRef, instanceRef }
, render: render this <$> React.getState this
}
where
render this { domRef, instanceRef }
= React.DOM.div
[]
-- 1. DOM ref using createRef
[ React.DOM.input
[ Props._type "text"
, Props.ref domRef
]
-- 2. DOM ref using callback
, React.DOM.div
[ Props.ref \ref -> do
-- Do something here
]
[ ]
-- 3. Instance ref using createRef
, React.createLeafElement anotherClass
{ ref: instanceRef
}
-- 4. Instance ref using callback
, React.createLeafElement anotherClass
{ ref: \ref -> do
-- Do something here
}
]
The challenge therefore is to find a nice API that works in all these cases.
It's also worth noting that in React you get slightly different results depending on how you create a ref: with createRef
you'll get something like { current: <ref> }
but with a callback you'll get <ref>
directly. I generally assumed we'd want to keep this distinction.
Ref a
I started with an approach along the same lines as @ethul tried in the hooks PR here: https://github.com/purescript-contrib/purescript-react/pull/159/files?file-filters%5B%5D=.js&file-filters%5B%5D=.purs#diff-039a468442c8f8ed540e0e4941930e29
This setup works really well for the first three cases, provided we don't mind having separate Props.ref
and Props.callbackRef
functions for DOM elements (I think this is fine). However, it doesn't immediately work for props passed to React classes since either ref
needs to be one of two distinct types or we need to support a new reserved prop called callbackRef
. So, I tried...
callbackRef
propThis approach would give us something like:
type ReservedReactPropFields r =
( key :: String
, ref :: Ref ReactInstance
, callbackRef :: SyntheticEventHandler (Nullable ReactInstance)
| r
)
I think this is a misleading type since it suggests to the user that they can pass both ref
and callbackRef
at once, which shouldn't be supported. Additionally we'd have to put in some code to intercept the callbackRef
prop and rename it to ref
before it gets to React, and it all gets very messy.
I really wanted to be able to write something like
type ReservedReactPropFields ref r =
( key :: String
, ref :: ref
| r
)
and then elsewhere add a constraint like IsReactRef ref => ...
and make the relevant types (e.g. Ref ReactInstance
and Ref (Nullable ReactInstance -> Effect Unit)
) instances of this class. Sadly neither I nor @LiamGoodacre could get this to work at all.
Finally I tried just using a sum type to represent the possibilities. For example:
data InstanceRef
= InstanceRefObject ReactInstance
| InstanceRefCallback (Nullable ReactInstance -> Effect Unit)
To do this we'd have to add helper functions to be used like so:
-- 3. Instance ref using createRef
, React.createLeafElement anotherClass
{ ref: Ref.withRef instanceRef
}
-- 4. Instance ref using callback
, React.createLeafElement anotherClass
{ ref: Ref.withCallbackRef \ref -> do
-- Do something here
}
Even if we do this, the ref
prop being part of a row means I can't see any good way to unwrap the type later so that the inner ref or callback can be passed to React. (I looked at Record.modify
etc but the field is optional and so I don't think it's possible to modify it only if it exists? At any rate, we'd still have to repeat that on createElement
, createLeafElement
and all the other similar functions.)
I've very possibly missed some type-level trickery that might help resolve this problem. Or, at the other end of the spectrum, I could move more of this logic into the FFI (for example, I could do the InstanceRef
sum type unwrapping on the JS side). This seems it should be a last resort though!
Alternatively we could:
createRef
- but that would be very opinionatedThe only other thing I can think of would be to add new functions like createElementWithRef
and createLeafElementWithRef
and just pass the ref as a function argument; at that point I think we could use an IsReactRef
typeclass without issue. This would be a fairly big departure from the current row type implementation, though; and we might then want to reconsider how reserved props are treated in general.
Thoughts most welcome :smile:
In my local projects when dealing with FFI overloading like this, I usually use an opaque foreign data type with smart constructors that are coercions under the hood. Maybe something like:
foreign import data RefHandler :: Type -> Type
foreign import fromEffect :: forall a. (a -> Effect Unit) -> RefHandler a
foreign import fromEffectFn :: (EffectFn1 a Unit) -> RefHandler a
foreign import fromRef :: Ref a ->RefHandler a
@elliotdavies Thanks for working out the above problem summary and proposed solutions. It is very much appreciated. I am inclined to agree with @natefaubion that using the opaque foreign type along with functions for the appropriate coercions is a good way to go. Would you be open to trying something like this?
@natefaubion @ethul Cheers for getting back to me. This approach sounds like it should work - I'll certainly give it a go. I'm away for the next three weeks but I'll see about it once I'm back 😄
@natefaubion @ethul Sorry for the lengthy delay! I finally got round to this and have submitted a PR for discussion.
I was doing some exploratory work around adding support for React's
createRef
API when I noticed a lot of similar work had already been done as part of the hooks PR: https://github.com/purescript-contrib/purescript-react/pull/159/files#diff-d96e307e87fe77fec439e08ec13d4947I'm not sure what the status of that PR is, but how do we feel about introducing the
createRef
part of it separately? (Probably including the newRef a
type, the accompanying functions, and theForwardRef
stuff.) I'd certainly be keen to help with this.