Skip to content

refs #100

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

Merged
merged 8 commits into from
Oct 11, 2017
Merged

refs #100

Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bower.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
"dependencies": {
"purescript-eff": "^3.0.0",
"purescript-prelude": "^3.0.0",
"purescript-unsafe-coerce": "^3.0.0"
"purescript-unsafe-coerce": "^3.0.0",
"purescript-maybe": "^3.0.0",
"purescript-nullable": "^3.0.0"
},
"devDependencies": {
"purescript-console": "^3.0.0",
Expand Down
24 changes: 24 additions & 0 deletions src/React.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,30 @@ function getChildren(this_) {
}
exports.getChildren = getChildren;

function readRefImpl (this_) {
return function(name) {
return function() {
var refs = this_.refs || {};
return refs[name];
}
}
}
exports.readRefImpl = readRefImpl;

function writeRef(this_) {
return function(name) {
return function(node) {
return function() {
var refs = this_.refs || {};
refs[name] = node;
this_.refs = refs;
return {};
}
}
}
}
exports.writeRef = writeRef;

function writeState(this_) {
return function(state){
return function(){
Expand Down
26 changes: 26 additions & 0 deletions src/React.purs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module React
, ReactRefs

, Refs
, Ref

, Render
, GetInitialState
Expand All @@ -43,6 +44,8 @@ module React

, getProps
, getRefs
, readRef
, writeRef
, getChildren

, readState
Expand All @@ -65,7 +68,10 @@ module React
) where

import Prelude

import Control.Monad.Eff (kind Effect, Eff)
import Data.Maybe (Maybe)
import Data.Nullable (Nullable, toMaybe)
import Unsafe.Coerce (unsafeCoerce)

-- | Name of a tag.
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment here?


-- | Read named ref from Refs
foreign import readRefImpl :: forall props state access eff.
ReactThis props state ->
String ->
Eff (refs :: ReactRefs (read :: Read | access) | eff) (Nullable Ref)

readRef :: forall props state access eff.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add comments to these as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ReactThis props state ->
String ->
Eff (refs :: ReactRefs (read :: Read | access) | eff) (Maybe Ref)
readRef this name = toMaybe <$> readRefImpl this name

foreign import writeRef :: forall props state access eff.
ReactThis props state ->
String ->
Ref ->
Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit

-- | Read the component children property.
foreign import getChildren :: forall props state eff.
ReactThis props state ->
Expand Down
18 changes: 17 additions & 1 deletion src/React/DOM/Props.purs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
module React.DOM.Props where

import React (Event, EventHandlerContext, KeyboardEvent, MouseEvent, handle)
import Control.Monad.Eff (Eff)
import Control.Monad.Eff.Unsafe (unsafePerformEff)
import Prelude (Unit, (<<<))
import React (Event, EventHandlerContext, KeyboardEvent, MouseEvent, ReactRefs, Ref, Write, handle)

foreign import data Props :: Type

Expand Down Expand Up @@ -297,6 +300,19 @@ radioGroup = unsafeMkProps "radioGroup"
readOnly :: Boolean -> Props
readOnly = unsafeMkProps "readOnly"

ref :: String -> Props
ref = unsafeMkProps "ref"

-- | You can use `writeRef` to store a reference on `Refs`.
-- | ``` purescrript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note: should be purescript

-- | div [ refCb (writeRef this "inputEl") ] [...]
-- | ```
refCb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

:: forall access eff
. (Ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ethul ethul Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks!

-> Props
refCb cb = unsafeMkProps "ref" (unsafePerformEff <<< cb)

rel :: String -> Props
rel = unsafeMkProps "rel"

Expand Down