This repository has been archived by the owner on Dec 13, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 141
More setState checks #26
Merged
LPGhatguy
merged 9 commits into
Roblox:master
from
AmaranthineCodices:more-setState-checks
Feb 17, 2018
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9396366
fix a tiny potential error in unit testing
AmaranthineCodices bae80f1
disallow setting state in all lifecycle hooks
AmaranthineCodices 3732dca
throw in didMount
AmaranthineCodices ffa15df
remove hacky force updating in tests
AmaranthineCodices d246b1d
Allow setState in didMount, didUpdate
AmaranthineCodices cc7ed18
Note about calling setState in didMount or didUpdate
AmaranthineCodices 1b53a5c
test shouldUpdate more thoroughly
AmaranthineCodices ed1cd2a
more descriptive error message
AmaranthineCodices ed60cbb
add a newline
AmaranthineCodices File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,10 @@ return function() | |
}) | ||
end | ||
|
||
function InitComponent:render() | ||
return 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. This is an interesting artifact of a test that's only as coarse as "this high level call should throw". I wonder if there's a way to make these kinds of tests more robust in the future. |
||
|
||
local initElement = Core.createElement(InitComponent) | ||
|
||
expect(function() | ||
|
@@ -207,6 +211,65 @@ return function() | |
end).to.throw() | ||
end) | ||
|
||
it("should throw when called in shouldUpdate", function() | ||
local TestComponent = Component:extend("TestComponent") | ||
|
||
local triggerTest | ||
|
||
function TestComponent:init() | ||
triggerTest = function() | ||
self:setState({ | ||
a = 1 | ||
}) | ||
end | ||
end | ||
|
||
function TestComponent:render() | ||
return nil | ||
end | ||
|
||
function TestComponent:shouldUpdate() | ||
self:setState({ | ||
a = 1 | ||
}) | ||
end | ||
|
||
local testElement = Core.createElement(TestComponent) | ||
|
||
expect(function() | ||
Reconciler.reify(testElement) | ||
triggerTest() | ||
end).to.throw() | ||
end) | ||
|
||
it("should throw when called in willUpdate", function() | ||
local TestComponent = Component:extend("TestComponent") | ||
local forceUpdate | ||
|
||
function TestComponent:init() | ||
forceUpdate = function() | ||
self:_forceUpdate() | ||
end | ||
end | ||
|
||
function TestComponent:render() | ||
return nil | ||
end | ||
|
||
function TestComponent:willUpdate() | ||
self:setState({ | ||
a = 1 | ||
}) | ||
end | ||
|
||
local testElement = Core.createElement(TestComponent) | ||
|
||
expect(function() | ||
Reconciler.reify(testElement) | ||
forceUpdate() | ||
end).to.throw() | ||
end) | ||
|
||
it("should throw when called in willUnmount", function() | ||
local TestComponent = Component:extend("TestComponent") | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
An interesting implication here: the state of your parent can't have its state updated while you're in the middle of updating. Calling a function in your
didUpdate
callback that indirectly invokessetState
on a parent component would thus fail.I wonder if that's a case that we hit in real code, and if that's a bug!
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 will only throw an error if this setState indirection happens synchronously. If you spawn another thread and trigger the parent
setState
call there, or do it from an event connection, there won't be an error.Since most use-cases I can see for calling
setState
indidUpdate
involve some sort of blocking operation (that stalls out the entire render until it's complete if it's done synchronously anyway), I don't think this is a major issue.Hypothetically this could be worked around by deferring
didUpdate
invocations until after the entire tree has re-rendered, parents and all, but I don't think this is worth the effort.