refs #100

coot · 2017-06-27T22:21:58Z

React.DOM.Props.ref - set ref property (string)
React.readRef - read ref through Foreign

String refs are not yet deprecated, but they might be at some point and React-15.6.1 docs do not event mention them. Nevertheless we should support them.

I'd be keen to change readRef type to

readRefs :: String -> Foreign -> Eff (refs :: ReactRef (read :: Read | access) | eff) (Either MultipleErrors Node)

i.e.

run runExcept
use Foreign directly

This way it will be also use able if one sets a ref using a callback. Then on could pass toForeigh this and get back the ref. Setting callback refs is not included in the patch but I'd be happy to find a solution for that - maybe with a more fresh mind in the morning :)

* React.DOM.Props.ref - set ref property (string) * React.readRef - read ref through Foreign

paf31 · 2017-07-01T19:00:58Z

I think this means adding a dependency on foreign which comes with a lot of dependencies, whereas react right now has almost none. I'd prefer to avoid the dependency if possible.

coot · 2017-07-08T09:07:37Z

The other option is to use Refs instead of Foriegn and to provide safe way of writing a ref to Refs and reading it from there (as a Maybe). Then we could also support callback arguments for the ref property which would write to Refs.

coot · 2017-07-08T12:17:32Z

now it only introduces purescript-dom dependency, which anyway will be in any project
I think we could be less conservative and use HTMLElement instead of Node type. Storing an attribute node (which is a Node but not Element), is possible but not something usable.
we could be more conservative on effects that we allow in refCb, for example (refs :: ReactRef (write :: Write | access), state :: ReactState () | eff)

ethul · 2017-07-08T12:37:10Z

I think this looks good. I want to take a bit closer look when I am at my computer. However, one question is regarding readRefImpl. Wouldn't this return undefined instead of null? There is a purescript-undefinable module. Thanks for working on this!

…

On Sat, Jul 8, 2017 at 08:17 Coot ***@***.***> wrote: - now it only introduces purescript-dom dependency, which anyway will be in any project - I think we could be less conservative and use HTMLElement instead of Node type. Storing an attribute node (which is a Node but not Element), is possible but not something usable. - we could be more conservative on effects that we allow in refCb, for example (refs :: ReactRef (write :: Write | access), state :: ReactState () | eff), there is no point on reading the state, and writting it will actually raise an exception — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#100 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYy5w-4Ytto8S6F_NCE57Tl29AHAebks5sL3NcgaJpZM4OHRBs> .

coot · 2017-07-08T12:49:37Z

purescript-nullable is using == null to check if something is null or not and undefined == null returns true. TherefCb is called with null (when a component unmounts), so it might be set to null. Undefinable is using === undefined so this will not detect a null. So Nullable looks safer to me.

ethul · 2017-07-08T12:50:46Z

Gotcha. Thanks for confirming

…

On Sat, Jul 8, 2017 at 08:49 Coot ***@***.***> wrote: purescript-nullable is using == null to check if something is null or not and undefined == null returns true. TherefCb is called with null, so it might be set to null. Undefinable <https://github.com/ethul/purescript-undefinable/blob/master/src/Data/Undefinable.js#L6> is using === undefined so this will not detect a null. So Nullable looks safer to me. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#100 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYy4YUP6Ua2sv42-vGpmf6OLp82uA_ks5sL3rhgaJpZM4OHRBs> .

paf31 · 2017-07-08T23:53:20Z

Unfortunately, dom still pulls in quite a few dependencies. I'd really like to avoid adding more dependencies here if we can - I really like that all you need is prelude and eff.

I'll have a think about some options.

coot · 2017-07-09T06:35:05Z

We could make refs polymorphic in the type. That would leave choice weather use Foreign or HTMLElement or a some more specific type like HTMLInputElement (Foreign seems the safest and most generic type though.)

Since this library is oriented towards the web, a requirement of purescript-dom or even purescript-foreign looks really minimal to me; purescript-react-dom already requires purescript-dom (and thus purescript-foreign).

ethul · 2017-07-09T16:18:07Z

After thinking about this further. I agree with @paf31. I think if we can avoid depending on purescript-dom, it would be ideal. For example, I don't think we'd necessarily need purescript-dom in a react native app.

ethul · 2017-07-11T01:26:41Z

src/React/DOM/Props.purs

+-- | ```
+refCb
+  :: forall access eff
+   . (Node -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)


Just a quick note, would it make sense for this to be Nullable Node?
https://facebook.github.io/react/docs/refs-and-the-dom.html#caveats

Just a quick note: I am wondering if perhaps refCb should be called ref since it is the encourage usage in React. Maybe the current ref prop can be called refString or something along those lines. What do you think?

ethul · 2017-08-29T23:11:48Z

I think it would be very useful to merge in this PR. However, I do think it would be best to avoid the DOM dependency. Would it make sense to replace Node with just a ref? E.g.,

ref :: forall access eff ref. (Nullable ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)

paf31 · 2017-09-02T00:53:15Z

But wouldn't the user be able to use such a function unsafely?

Do we even use the ref type anywhere? What about just creating a new foreign data type, and then the user can coerce it if they want to pull in dom.

ethul · 2017-09-02T00:56:54Z

Indeed it could be used in an unsafe way. I think a new foreign type would work.

…

On Fri, Sep 1, 2017 at 20:53 Phil Freeman ***@***.***> wrote: But wouldn't the user be able to use such a function unsafely? Do we even use the ref type anywhere? What about just creating a new foreign data type, and then the user can coerce it if they want to pull in dom. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#100 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVYy1AKFKj1l_XrG5B5Mrw-QemS5gkyks5seKb7gaJpZM4OHRBs> .

Still adds two small dependencies * purescript-maybe * purescript-nullable

paf31 · 2017-09-05T19:10:20Z

src/React.purs

@@ -294,6 +300,26 @@ foreign import getRefs :: forall props state access eff.
  ReactThis props state ->
  Eff (refs :: ReactRefs (read :: Read | access) | eff) Refs

+foreign import data Ref :: Type


Could you please add a comment here?

paf31 · 2017-09-05T19:11:27Z

src/React/DOM/Props.purs

+-- | ``` purescrript
+-- | div [ refCb (writeRef this "inputEl") ] [...]
+-- | ```
+refCb


I don't like this name so much, what about something like withRef?

I actually I don't like it either, withRef sounds much better :)

paf31 · 2017-09-05T19:12:03Z

src/React.purs

+  String ->
+  Eff (refs :: ReactRefs (read :: Read | access) | eff) (Nullable Ref)
+
+readRef :: forall props state access eff.


Could you please add comments to these as well?

Ref type should also be documented, and advising the user to use unsafeCoerce make me feel I am doing something wrong, so either we provide a third library with Node -> Ref and Ref -> Node safe functions (purescript-react-refs) or we can include that in purescript-react-dom, which already depends on purescript-dom.
There are pros and cons of both, but either will provide a safe way for the basic operation.

By the way: using zypher & webpack (with uglify plugin) on purescript-isomorphic-react-example, which is pulling DOM for very basic stuff (access to location, history, search the mounting point in the DOM), pulls ~2.3KB of data from purescript-dom, which is ~0.5% of whole bundle (~400KB, which gzips to 94KB). zypher, to some extend can remove things from foreign modules that's why the overall code from DOM that lands in the bundle is pretty minimal.

In my opinion, I think purescript-react-dom would be a nice spot for functions that convert between Ref and Node. Similarly, in a purescript-react-native library, there could be functions that convert between Ref and something native.

ethul · 2017-08-29T23:03:17Z

src/React/DOM/Props.purs

+-- | ```
+refCb
+  :: forall access eff
+   . (Node -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)


Just a quick note: I am wondering if perhaps refCb should be called ref since it is the encourage usage in React. Maybe the current ref prop can be called refString or something along those lines. What do you think?

ethul · 2017-09-06T00:54:17Z

src/React/DOM/Props.purs

+-- | ```
+refCb
+  :: forall access eff
+   . (Ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)


Just as a note, I think this may need to be

(Nullable Ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)

From react docs:

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts.

And this is done to avoid memory leaks (DOM references used to leak in old IEs). If we write the ref only if it's not null, you'll prevent the original mechanism to prevent memory leaks.

So if we change this type signature (which I do like, since things are explicitly typed then) then we also need to change writeRef :: forall p s. ReactThis p s -> String -> Nullable Ref -> Eff ...

Agreed. I think it makes sense to write a Nullable Ref.

coot · 2017-09-30T10:45:09Z

I added comments and renamed the function withRefs. Another PR will go topurescritp-react-dom which adds refToNode which is mentioned in the comments.

purescript-contrib/purescript-react#100

ethul · 2017-09-30T12:04:48Z

src/React/DOM/Props.purs

+-- | ```
+withRef
+  :: forall access eff
+   . (Ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)


Looks great! Quick question: Did we want this to be Nullable Ref? Or are we thinking that the possibility the ref is null would be handled elsewhere.
https://facebook.github.io/react/docs/refs-and-the-dom.html#caveats

Good point. Having here Nullable Ref will be more explicit, so I prefer this. Then in the React.DOM.refToNode we will not need to use unsafeCoerce

We will only need to add a comment, that if one is attaching the ref to ReactThis one should do that using the Nullable Ref rather than Ref itself.

Great! Thanks!

ethul · 2017-10-04T23:08:29Z

src/React/DOM/Props.purs

+ref = unsafeMkProps "ref"
+
+-- | You can use `writeRef` to store a reference on `Refs`.
+-- | ``` purescrript


Quick note: should be purescript

ethul · 2017-10-04T23:09:45Z

Looking great! @paf31 Do you have any other comments for this one?

* Added refToNode purescript-contrib/purescript-react#100 * refToNode :: Nullable Ref -> Maybe Node purescript-contrib/purescript-react@1b0d542

Marcin Szamotulski added 3 commits June 28, 2017 00:10

refs

088bbf7

* React.DOM.Props.ref - set ref property (string) * React.readRef - read ref through Foreign

readRef from Foreign rather than Refs

90ea09b

React.DOM.Props.refCb

796debd

coot changed the title ~~[WIP] refs~~ refs Jun 28, 2017

readRef and writeRef through ffi

aaa1ba8

ethul reviewed Jul 11, 2017

View reviewed changes

Remove DOM dependency, use foreign data type for Ref

00d51f6

Still adds two small dependencies * purescript-maybe * purescript-nullable

paf31 reviewed Sep 5, 2017

View reviewed changes

ethul reviewed Sep 6, 2017

View reviewed changes

Rename: withRef && added comments.

dc2b66a

coot pushed a commit to coot/purescript-react-dom that referenced this pull request Sep 30, 2017

Added refToNode

32c7419

purescript-contrib/purescript-react#100

coot mentioned this pull request Sep 30, 2017

Added refToNode purescript-contrib/purescript-react-dom#8

Merged

ethul reviewed Sep 30, 2017

View reviewed changes

Marcin Szamotulski added 2 commits September 30, 2017 18:58

Nullable Ref in withRefs

1b0d542

Merge branch 'master' into refs

5395879

ethul reviewed Oct 4, 2017

View reviewed changes

ethul merged commit 03d52a5 into purescript-contrib:master Oct 11, 2017

ethul mentioned this pull request Oct 11, 2017

It doesn't appear to be possible to do much with Refs #78

Closed

ethul pushed a commit to purescript-contrib/purescript-react-dom that referenced this pull request Oct 11, 2017

Added refToNode (#8)

583bdd7

* Added refToNode purescript-contrib/purescript-react#100 * refToNode :: Nullable Ref -> Maybe Node purescript-contrib/purescript-react@1b0d542

coot deleted the refs branch October 12, 2017 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refs #100

refs #100

coot commented Jun 27, 2017

paf31 commented Jul 1, 2017

coot commented Jul 8, 2017

coot commented Jul 8, 2017 •

edited

Loading

ethul commented Jul 8, 2017 via email

coot commented Jul 8, 2017 •

edited

Loading

ethul commented Jul 8, 2017 via email

paf31 commented Jul 8, 2017

coot commented Jul 9, 2017

ethul commented Jul 9, 2017

ethul Jul 11, 2017

coot Jul 11, 2017

ethul Aug 29, 2017

ethul commented Aug 29, 2017

paf31 commented Sep 2, 2017

ethul commented Sep 2, 2017 via email

paf31 Sep 5, 2017

paf31 Sep 5, 2017

coot Sep 5, 2017

paf31 Sep 5, 2017

coot Sep 5, 2017

ethul Sep 6, 2017

ethul Aug 29, 2017

ethul Sep 6, 2017

coot Sep 6, 2017

ethul Sep 6, 2017

coot commented Sep 30, 2017 •

edited

Loading

ethul Sep 30, 2017 •

edited

Loading

coot Sep 30, 2017

coot Sep 30, 2017

ethul Sep 30, 2017

ethul Oct 4, 2017

ethul commented Oct 4, 2017

refs #100

refs #100

Conversation

coot commented Jun 27, 2017

paf31 commented Jul 1, 2017

coot commented Jul 8, 2017

coot commented Jul 8, 2017 • edited Loading

ethul commented Jul 8, 2017 via email

coot commented Jul 8, 2017 • edited Loading

ethul commented Jul 8, 2017 via email

paf31 commented Jul 8, 2017

coot commented Jul 9, 2017

ethul commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethul commented Aug 29, 2017

paf31 commented Sep 2, 2017

ethul commented Sep 2, 2017 via email

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

Choose a reason for hiding this comment

coot commented Sep 30, 2017 • edited Loading

ethul Sep 30, 2017 • edited Loading

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

ethul commented Oct 4, 2017

coot commented Jul 8, 2017 •

edited

Loading

coot commented Jul 8, 2017 •

edited

Loading

coot commented Sep 30, 2017 •

edited

Loading

ethul Sep 30, 2017 •

edited

Loading