-
Notifications
You must be signed in to change notification settings - Fork 143
createRef API #92
createRef API #92
Changes from 7 commits
4e773ef
82b2330
72b907c
35a9df7
d002373
e2f452f
b624801
9ce1d84
3cda6f7
1f86eb1
878ecc5
598af40
6f770ab
8c6cb36
4614af4
3378d4a
336db31
9de73c7
b139cc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,13 @@ If `children` contains more than one child, `oneChild` function will throw an er | |
|
||
If `children` is `nil` or contains no children, `oneChild` will return `nil`. | ||
|
||
### Roact.createRef | ||
``` | ||
Roact.createRef() -> Ref | ||
``` | ||
|
||
Creates a new reference object that can be used with [Roact.Ref](#roactref). | ||
|
||
## Constants | ||
|
||
### Roact.Children | ||
|
@@ -72,6 +79,10 @@ If you're writing a new functional or stateful element that needs to be used lik | |
Use `Roact.Ref` as a key into the props of a primitive element to receive a handle to the underlying Roblox Instance. | ||
|
||
```lua | ||
Roact.createElement("Frame", { | ||
[Roact.Ref] = objectReference, | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This example seems sort of incomplete -- we should break the functional and object ref examples into two separate blocks and label what they do! |
||
|
||
Roact.createElement("Frame", { | ||
[Roact.Ref] = function(rbx) | ||
print("Roblox Instance", rbx) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,18 @@ local function isPortal(element) | |
return element.component == Core.Portal | ||
end | ||
|
||
--[[ | ||
Sets the value of a reference to a new rendered object. | ||
Correctly handles both function-style and object-style refs. | ||
]] | ||
local function applyRef(ref, newRbx) | ||
if type(ref) == "table" then | ||
ref.current = newRbx | ||
else | ||
ref(newRbx) | ||
end | ||
end | ||
|
||
local Reconciler = {} | ||
|
||
Reconciler._singleEventManager = SingleEventManager.new() | ||
|
@@ -93,8 +105,10 @@ function Reconciler.unmount(instanceHandle) | |
|
||
-- Kill refs before we make changes, since any mutations past this point | ||
-- aren't relevant to components. | ||
if element.props[Core.Ref] then | ||
element.props[Core.Ref](nil) | ||
local ref = element.props[Core.Ref] | ||
|
||
if ref then | ||
applyRef(ref, nil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move this condition into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That'd help clean some stuff up, yeah! |
||
end | ||
|
||
for _, child in pairs(instanceHandle._children) do | ||
|
@@ -173,8 +187,9 @@ function Reconciler._mountInternal(element, parent, key, context) | |
rbx.Parent = parent | ||
|
||
-- Attach ref values, since the instance is initialized now. | ||
if element.props[Core.Ref] then | ||
element.props[Core.Ref](rbx) | ||
local ref = element.props[Core.Ref] | ||
if ref then | ||
applyRef(ref, rbx) | ||
end | ||
|
||
return { | ||
|
@@ -323,7 +338,7 @@ function Reconciler._reconcileInternal(instanceHandle, newElement) | |
|
||
-- Cancel the old ref before we make changes. Apply the new one after. | ||
if refChanged and oldRef then | ||
oldRef(nil) | ||
applyRef(oldRef, nil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to update the branch on line 349 that applies |
||
end | ||
|
||
-- Update properties and children of the Roblox object. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,39 @@ | ||
return function() | ||
local Core = require(script.Parent.Core) | ||
local Reconciler = require(script.Parent.Reconciler) | ||
local createRef = require(script.Parent.createRef) | ||
|
||
it("should mount booleans as nil", function() | ||
local booleanReified = Reconciler.mount(false) | ||
expect(booleanReified).to.never.be.ok() | ||
end) | ||
|
||
it("should handle object references properly", function() | ||
local objectRef = createRef() | ||
local element = Core.createElement("StringValue", { | ||
[Core.Ref] = objectRef, | ||
}) | ||
|
||
local handle = Reconciler.mount(element) | ||
expect(objectRef.current).to.be.ok() | ||
Reconciler.unmount(handle) | ||
expect(objectRef.current).to.never.be.ok() | ||
end) | ||
|
||
it("should handle function references properly", function() | ||
local currentRbx | ||
|
||
local function ref(rbx) | ||
currentRbx = rbx | ||
end | ||
|
||
local element = Core.createElement("StringValue", { | ||
[Core.Ref] = ref, | ||
}) | ||
|
||
local handle = Reconciler.mount(element) | ||
expect(currentRbx).to.be.ok() | ||
Reconciler.unmount(handle) | ||
expect(currentRbx).to.never.be.ok() | ||
end) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test that makes sure that refs can be unattached and attached through |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
--[[ | ||
Provides an API for acquiring a reference to a reified object. This | ||
API is designed to mimic React 16.3's createRef API. | ||
|
||
See: | ||
* https://reactjs.org/docs/refs-and-the-dom.html | ||
* https://reactjs.org/blog/2018/03/29/react-v-16-3.html#createref-api | ||
]] | ||
|
||
local refMetatable = { | ||
__tostring = function(self) | ||
return ("RoactReference(%q)"):format(tostring(self.current)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about it now, do you think we should just format these with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true - I'm used to quoting everything, but in this case it doesn't do much good! Fixed now. |
||
end, | ||
} | ||
|
||
return function() | ||
return setmetatable({ | ||
current = nil, | ||
}, refMetatable) | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
return function() | ||
local createRef = require(script.Parent.createRef) | ||
|
||
it("should create refs", function() | ||
expect(createRef()).to.be.ok() | ||
end) | ||
|
||
it("should support tostring on refs", function() | ||
local ref = createRef() | ||
expect(tostring(ref)).to.equal("RoactReference(\"nil\")") | ||
end) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're going to test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair, done! |
||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the guie changes to a new PR? There are quite a few more things we need to catch across the docs, and I want to let the API simmer a bit before recommending it as the new default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, done!