-
Notifications
You must be signed in to change notification settings - Fork 141
Conversation
Added createContext, provide, and consume APIs to Roact.
Removes provide and consume from the new Roact context API, so that createContext returns a Provider and Consumer.
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.
Looking good!
Left some comments.
We'll also want to port the code to use the new context API defined in #247 once that lands and make sure everything still works.
src/createContext.lua
Outdated
end | ||
|
||
function Provider:didUpdate() | ||
self.updateValue(self.props.value) |
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.
Let's check that prevProps.value ~= self.props.value
before firing this method. This will let us avoid firing updates when we don't need to.
src/createContext.lua
Outdated
end | ||
|
||
function Provider:render() | ||
return oneChild(self.props[Children]) |
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.
We should use a fragment here instead! We added them in 1.0 to improve cases like this, and will let providers have multiple children instead of just one.
src/createContext.lua
Outdated
local PureComponent = require(script.Parent.PureComponent) | ||
|
||
local function createProvider(context) | ||
local Provider = PureComponent:extend("Provider") |
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.
Let's use a regular component here. Because this component is always going to have children, the shouldUpdate
check provided by PureComponent
is almost always going to fail.
In general, we should prefer function components or Component
by default, and fall back to PureComponent
with profiling if we have performance issues.
src/createContext.lua
Outdated
end | ||
|
||
function Consumer:render() | ||
assert(type(self.props.render) == "function", "Consumer expects a `render` function") |
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.
We should be able to use Roact's validateProps
feature here instead of asserting in render
!
src/createContext.lua
Outdated
end | ||
|
||
function Consumer:willUnmount() | ||
if self.disconnect then |
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 be a little bit more explicit with this check, as in
if self.disconnect then | |
if self.disconnect ~= nil then |
It's unlikely that disconnect
will be false
, but we've been trying to encourage that explicitness when the exact type involved is unclear.
src/createContext.lua
Outdated
end | ||
|
||
function Context:__tostring() | ||
return tostring(self.defaultValue) |
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.
This is kind of a strange __tostring
!
Can we return a fixed string here like RoactContext
?
src/createContext.spec.lua
Outdated
local didRender = false | ||
local context = createContext("Test") | ||
|
||
local Listener = PureComponent:extend("Listener") |
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.
Instead of a PureComponent
, this should just be a function component for simplicity.
src/createContext.spec.lua
Outdated
|
||
expect(function() | ||
local tree = noopReconciler.mountVirtualTree(element, nil, "Provide Tree") | ||
noopReconciler.unmountVirtualTree(tree) |
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.
We don't need to call unmountVirtualTree
. We should limit the scope of what's inside this expect
block so that we don't erroneously pass this test when something else throws.
noopReconciler.unmountVirtualTree(tree) | ||
|
||
expect(foundValue).to.equal("Test") | ||
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.
I think it would be more robust to use Roact's internal createSpy
method to create a spy function to use for the render function. This lets us assert how many times it was called, as well as all of the arguments passed.
Notably, this lets us catch errors like:
- We invoke the function twice and the first one is incorrect
- We pass extra arguments to the function
expect(foundValue).to.equal("ThirdTest") | ||
|
||
noopReconciler.unmountVirtualTree(tree) | ||
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.
This test can also be rewritten in terms of Roact's spies, which might be more readable.
-Use fragment instead of oneChild -Use spies for testing -Use Component over PureComponent -Check for duplicates in didUpdate -nil check disconnect
Merge with current Roblox master
Updates the createContext API to internally use the new internal Context API.
Adds createContext, provide, and consume APIs to Roact.
Components that consume Contexts automatically update when the Context updates, wherever they are in the tree.
Closes #4 .
Checklist before submitting:
CHANGELOG.md