Add support for `createRef` API #172

elliotdavies · 2019-07-01T18:12:07Z

Fixes #167 by adding support for React's createRef API. Also distinguishes between DOM refs and React instance refs.

I've created a branch of purescript-react-example to showcase the different combinations of props, callbacks and ref types (see here in particular).

The ReactInstance type probably isn't in the right place; I can move it somewhere else. (React.purs feels better but that causes an import cycle.)

Otherwise the only thing I dislike is having to check ref.current to see whether the item is a ref, or an object with the shape { current: <ref> } (see https://reactjs.org/docs/refs-and-the-dom.html#accessing-refs). I'm not sure I see a way around this, though.

ethul · 2019-07-03T22:11:06Z

Thanks for your PR! I will have to dig into this, might take me a bit of time. Apologies for that, but I will get to it.

Shou

Very nice work, just one small bug

src/React/Ref.js

LiamGoodacre

Looks good to me, just a quick note on the getCurrentRef_ impl.

src/React/Ref.js

ethul · 2019-07-25T12:58:53Z

src/React/DOM/Props.purs

@@ -20,6 +19,7 @@ import React.SyntheticEvent
  , SyntheticUIEvent
  , SyntheticWheelEvent
  )
+import Web.HTML.HTMLElement (HTMLElement)


Just a quick note. This is coming from purescript-web-html, but I didn't notice the dependency in the bower file. Is it being pulled in from another dependency?

On this note, I am unsure if we want this as a dependency since runtimes like react native, etc., would not necessarily require it. So far we have been attempting to keep dependencies at a minimum. That being said, I am not completely opposed to adding the dependency, but wanted to open it up for discussion.

@ethul Well spotted; I think I forgot to push the change to the bower file. I had added "purescript-web-html": "^2.2.0" locally.

I suppose that we could have a local HTMLElement type here and not use the library version. It would save on the dependency, but on the other hand anyone using DOM refs would have to write a conversion function (i.e. unsafeCoerce), which might be annoying.

Thanks for adding the dependency.

ethul · 2019-08-02T12:50:21Z

src/React/DOM/Props.purs

@@ -894,8 +894,8 @@ onScrollCapture f = unsafeMkProps "onScrollCapture" (mkEffectFn1 f)
 onWheelCapture :: (SyntheticWheelEvent -> Effect Unit) -> Props
 onWheelCapture f = unsafeMkProps "onWheelCapture" (mkEffectFn1 f)

-ref :: (Nullable ReactRef -> Effect Unit) -> Props
-ref f = unsafeMkProps "ref" (mkEffectFn1 f)
+ref :: Ref.RefHandler HTMLElement -> Props


Is this always HTMLElement here? For example, on React Native, would this be something different?

Good question. I haven't used React Native myself, but I looked into this and as far as I can tell Native doesn't have refs - it seems to use something called Direct Manipulation instead. This does raise the question of why the React team put refs in the React library instead of ReactDOM; I'm not sure what the answer is.

ethul · 2019-08-02T14:33:50Z

Thanks for looking into this. You’re right that react native uses direct manipulation, but I believe one would still need to use the ref prop to do this. From the link: <View ref={component => this._root = component} {...this.props}>

…

On Fri, Aug 2, 2019 at 10:13 Elliot Davies ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/React/DOM/Props.purs <#172 (comment)> : > @@ -894,8 +894,8 @@ onScrollCapture f = unsafeMkProps "onScrollCapture" (mkEffectFn1 f) onWheelCapture :: (SyntheticWheelEvent -> Effect Unit) -> Props onWheelCapture f = unsafeMkProps "onWheelCapture" (mkEffectFn1 f) -ref :: (Nullable ReactRef -> Effect Unit) -> Props -ref f = unsafeMkProps "ref" (mkEffectFn1 f) +ref :: Ref.RefHandler HTMLElement -> Props Good question. I haven't used React Native myself, but I looked into this and as far as I can tell Native doesn't have refs - it seems to use something called Direct Manipulation <https://facebook.github.io/react-native/docs/direct-manipulation> instead. This does raise the question of why the React team put refs in the React library instead of ReactDOM; I'm not sure what the answer is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#172?email_source=notifications&email_token=AACVRSZVDQDALMMWD7MOU73QCQ6JJA5CNFSM4H4UYBH2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCANPRDI#discussion_r310150245>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACVRS2GSDCD4VVT4Q2IXBTQCQ6JJANCNFSM4H4UYBHQ> .

elliotdavies · 2019-08-02T14:46:41Z

@ethul Hmm, after a closer reading of that docs page I think you're correct. I would guess that ReactInstance is correct across web/native for a ref on a custom component, but presumably you get either a DOM node or some sort of native element node if you use a ref on a web/native primitive. In that case do you think it's best to have something like

ref :: Ref.RefHandler PrimitiveNode -> Props

and let the user coerce it as they see fit? This would also avoid theweb-html dependency. (Open to suggestions for a decent type name here!)

ethul · 2019-08-02T14:53:48Z

I suppose I am kind of leaning in that direction. However, I am open to further discussion on the matter. If we go this route, maybe the name could be NativeNode or ReactNode, or something like that? Thanks again for working on this.

…

On Fri, Aug 2, 2019 at 10:46 Elliot Davies ***@***.***> wrote: @ethul <https://github.com/ethul> Hmm, after a closer reading of that docs page I think you're correct. I would guess that ReactInstance is correct across web/native for a ref on a custom component, but presumably you get either a DOM node or some sort of native element node if you use a ref on a web/native primitive. In that case do you think it's best to have something like ref :: Ref.RefHandler PrimitiveNode -> Props and let the user coerce it as they see fit? This would also avoid the web-html dependency. (Open to suggestions for a decent type name here!) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#172?email_source=notifications&email_token=AACVRS5FFLE5WXNHJZAA4GLQCRCFFA5CNFSM4H4UYBH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3N6QQY#issuecomment-517728323>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACVRS26MAITFKBP4LZGKW3QCRCFFANCNFSM4H4UYBHQ> .

elliotdavies · 2019-08-06T13:05:40Z

@LiamGoodacre I've updated the FFI implementation such that callback refs are lifted into an object like { current: ref }, thereby making the .current property access safe.

@ethul I've updated the types to use NativeNode and removed the HTMLElement import. I think the React Native use case is compelling enough to go down this route.

src/React/Ref.purs

ethul · 2019-08-10T10:16:05Z

Thanks so much for making these updates. Looking great!

elliotdavies · 2019-08-10T11:18:09Z

@ethul No problem! Anything else you'd like doing to this before we can get it merged?

ethul · 2019-08-10T11:36:27Z

Thanks! All looks good to me

elliotdavies added 3 commits July 1, 2019 18:56

Get refs compiling

73ebb39

Tidy up

a469d23

Actually include Refs files, duh

8b39bdc

Shou suggested changes Jul 25, 2019

View reviewed changes

src/React/Ref.js Outdated Show resolved Hide resolved

Bugfix Ref.js

59987d8

LiamGoodacre reviewed Jul 25, 2019

View reviewed changes

src/React/Ref.js Outdated Show resolved Hide resolved

ethul reviewed Jul 25, 2019

View reviewed changes

Add bower.json file

5678e34

ethul reviewed Aug 2, 2019

View reviewed changes

elliotdavies added 5 commits August 6, 2019 12:14

HTMLElement -> NativeNode

81d138d

Lift callback refs into an object

b703b8f

Missed reference to HTMLElement

42c7274

Remove unneeded import

76316db

Comments

15ad056

ethul reviewed Aug 10, 2019

View reviewed changes

src/React/Ref.purs Outdated Show resolved Hide resolved

DOM -> Node

62106d3

ethul merged commit 4bf5a5a into purescript-contrib:master Aug 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `createRef` API #172

Add support for `createRef` API #172

elliotdavies commented Jul 1, 2019 •

edited

Loading

ethul commented Jul 3, 2019

Shou left a comment

LiamGoodacre left a comment •

edited

Loading

ethul Jul 25, 2019 •

edited

Loading

elliotdavies Jul 25, 2019

ethul Aug 2, 2019

ethul Aug 2, 2019

elliotdavies Aug 2, 2019

ethul commented Aug 2, 2019 via email

elliotdavies commented Aug 2, 2019

ethul commented Aug 2, 2019 via email

elliotdavies commented Aug 6, 2019

ethul commented Aug 10, 2019

elliotdavies commented Aug 10, 2019

ethul commented Aug 10, 2019

Add support for createRef API #172

Add support for createRef API #172

Conversation

elliotdavies commented Jul 1, 2019 • edited Loading

ethul commented Jul 3, 2019

Shou left a comment

Choose a reason for hiding this comment

LiamGoodacre left a comment • edited Loading

Choose a reason for hiding this comment

ethul Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

elliotdavies Jul 25, 2019

Choose a reason for hiding this comment

ethul Aug 2, 2019

Choose a reason for hiding this comment

ethul Aug 2, 2019

Choose a reason for hiding this comment

elliotdavies Aug 2, 2019

Choose a reason for hiding this comment

ethul commented Aug 2, 2019 via email

elliotdavies commented Aug 2, 2019

ethul commented Aug 2, 2019 via email

elliotdavies commented Aug 6, 2019

ethul commented Aug 10, 2019

elliotdavies commented Aug 10, 2019

ethul commented Aug 10, 2019

Add support for `createRef` API #172

Add support for `createRef` API #172

elliotdavies commented Jul 1, 2019 •

edited

Loading

LiamGoodacre left a comment •

edited

Loading

ethul Jul 25, 2019 •

edited

Loading