-
Notifications
You must be signed in to change notification settings - Fork 32
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
WIP: Introduce StoreUpdateGate
, for temporarily blocking store updates for its descendants
#7
Closed
Closed
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
fba16a7
Initial base for StoreUpdateGate
LPGhatguy 8a13eb9
Merge branch 'master' into update-blocker
LPGhatguy 88f54bb
Second pass at StoreUpdateGate, this should function
LPGhatguy caea0c0
Port signal tests from Rodux
LPGhatguy dc4f1e9
Only trigger a change on unblock if the state changed during the block
LPGhatguy 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
--[[ | ||
A limited, simple implementation of a Signal. | ||
|
||
Handlers are fired in order, and (dis)connections are properly handled when | ||
executing an event. | ||
]] | ||
|
||
local function immutableAppend(list, ...) | ||
local new = {} | ||
local len = #list | ||
|
||
for key = 1, len do | ||
new[key] = list[key] | ||
end | ||
|
||
for i = 1, select("#", ...) do | ||
new[len + i] = select(i, ...) | ||
end | ||
|
||
return new | ||
end | ||
|
||
local function immutableRemoveValue(list, removeValue) | ||
local new = {} | ||
|
||
for i = 1, #list do | ||
if list[i] ~= removeValue then | ||
table.insert(new, list[i]) | ||
end | ||
end | ||
|
||
return new | ||
end | ||
|
||
local Signal = {} | ||
|
||
Signal.__index = Signal | ||
|
||
function Signal.new() | ||
local self = { | ||
_listeners = {} | ||
} | ||
|
||
setmetatable(self, Signal) | ||
|
||
return self | ||
end | ||
|
||
function Signal:connect(callback) | ||
local listener = { | ||
callback = callback, | ||
disconnected = false, | ||
} | ||
|
||
self._listeners = immutableAppend(self._listeners, listener) | ||
|
||
local function disconnect() | ||
listener.disconnected = true | ||
self._listeners = immutableRemoveValue(self._listeners, listener) | ||
end | ||
|
||
return { | ||
disconnect = disconnect | ||
} | ||
end | ||
|
||
function Signal:fire(...) | ||
for _, listener in ipairs(self._listeners) do | ||
if not listener.disconnected then | ||
listener.callback(...) | ||
end | ||
end | ||
end | ||
|
||
return Signal |
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 |
---|---|---|
@@ -0,0 +1,114 @@ | ||
return function() | ||
local Signal = require(script.Parent.Signal) | ||
|
||
it("should construct from nothing", function() | ||
local signal = Signal.new() | ||
|
||
expect(signal).to.be.ok() | ||
end) | ||
|
||
it("should fire connected callbacks", function() | ||
local callCount = 0 | ||
local value1 = "Hello World" | ||
local value2 = 7 | ||
|
||
local callback = function(arg1, arg2) | ||
expect(arg1).to.equal(value1) | ||
expect(arg2).to.equal(value2) | ||
callCount = callCount + 1 | ||
end | ||
|
||
local signal = Signal.new() | ||
|
||
local connection = signal:connect(callback) | ||
signal:fire(value1, value2) | ||
|
||
expect(callCount).to.equal(1) | ||
|
||
connection:disconnect() | ||
signal:fire(value1, value2) | ||
|
||
expect(callCount).to.equal(1) | ||
end) | ||
|
||
it("should disconnect handlers", function() | ||
local callback = function() | ||
error("Callback was called after disconnect!") | ||
end | ||
|
||
local signal = Signal.new() | ||
|
||
local connection = signal:connect(callback) | ||
connection:disconnect() | ||
|
||
signal:fire() | ||
end) | ||
|
||
it("should fire handlers in order", function() | ||
local signal = Signal.new() | ||
local x = 0 | ||
local y = 0 | ||
|
||
local callback1 = function() | ||
expect(x).to.equal(0) | ||
expect(y).to.equal(0) | ||
x = x + 1 | ||
end | ||
|
||
local callback2 = function() | ||
expect(x).to.equal(1) | ||
expect(y).to.equal(0) | ||
y = y + 1 | ||
end | ||
|
||
signal:connect(callback1) | ||
signal:connect(callback2) | ||
signal:fire() | ||
|
||
expect(x).to.equal(1) | ||
expect(y).to.equal(1) | ||
end) | ||
|
||
it("should continue firing despite mid-event disconnection", function() | ||
local signal = Signal.new() | ||
local countA = 0 | ||
local countB = 0 | ||
|
||
local connectionA | ||
connectionA = signal:connect(function() | ||
connectionA:disconnect() | ||
countA = countA + 1 | ||
end) | ||
|
||
signal:connect(function() | ||
countB = countB + 1 | ||
end) | ||
|
||
signal:fire() | ||
|
||
expect(countA).to.equal(1) | ||
expect(countB).to.equal(1) | ||
end) | ||
|
||
it("should skip listeners that were disconnected during event evaluation", function() | ||
local signal = Signal.new() | ||
local countA = 0 | ||
local countB = 0 | ||
|
||
local connectionB | ||
|
||
signal:connect(function() | ||
countA = countA + 1 | ||
connectionB:disconnect() | ||
end) | ||
|
||
connectionB = signal:connect(function() | ||
countB = countB + 1 | ||
end) | ||
|
||
signal:fire() | ||
|
||
expect(countA).to.equal(1) | ||
expect(countB).to.equal(0) | ||
end) | ||
end |
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 |
---|---|---|
@@ -0,0 +1,56 @@ | ||
local Roact = require(script.Parent.Parent.Roact) | ||
|
||
local storeKey = require(script.Parent.storeKey) | ||
local Signal = require(script.Parent.Signal) | ||
|
||
local StoreUpdateGate = Roact.Component:extend("StoreUpdateGate") | ||
|
||
function StoreUpdateGate:init() | ||
local realStore = self._context[storeKey] | ||
|
||
if realStore == nil then | ||
error("StoreUpdateGate must be placed below a StoreProvider in the tree!") | ||
end | ||
|
||
self.realStore = realStore | ||
|
||
-- mockStore has a replaced version of 'changed' | ||
local mockStore = {} | ||
setmetatable(mockStore, { | ||
__index = realStore, | ||
}) | ||
|
||
mockStore.changed = Signal.new() | ||
|
||
self.mockStore = mockStore | ||
self._context[storeKey] = mockStore | ||
|
||
self.stateAtLastBlock = nil | ||
self.changedConnection = realStore.changed:connect(function(nextState, previousState) | ||
if self.props.shouldBlockUpdates then | ||
return | ||
end | ||
|
||
mockStore.changed:fire(nextState, previousState) | ||
end) | ||
end | ||
|
||
function StoreUpdateGate:didUpdate(oldProps) | ||
if self.props.shouldBlockUpdates ~= oldProps.shouldBlockUpdates then | ||
if self.props.shouldBlockUpdates then | ||
self.stateAtLastBlock = self.realStore:getState() | ||
else | ||
self.mockStore.changed:fire(self.realStore:getState(), self.stateAtLastBlock) | ||
end | ||
end | ||
end | ||
|
||
function StoreUpdateGate:willUnmount() | ||
self.changedConnection:disconnect() | ||
end | ||
|
||
function StoreUpdateGate:render() | ||
return Roact.oneChild(self.props[Roact.Children]) | ||
end | ||
|
||
return StoreUpdateGate |
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 |
---|---|---|
@@ -0,0 +1,89 @@ | ||
return function() | ||
local Roact = require(script.Parent.Parent.Roact) | ||
local Rodux = require(script.Parent.Parent.Rodux) | ||
|
||
local StoreProvider = require(script.Parent.StoreProvider) | ||
local connect = require(script.Parent.connect2) | ||
|
||
local StoreUpdateGate = require(script.Parent.StoreUpdateGate) | ||
|
||
it("should throw if there is no StoreProvider above it", function() | ||
local success, result = pcall(function() | ||
return Roact.mount(Roact.createElement(StoreUpdateGate)) | ||
end) | ||
|
||
expect(success).to.equal(false) | ||
expect(result:find("StoreUpdateGate")).to.be.ok() | ||
expect(result:find("StoreProvider")).to.be.ok() | ||
end) | ||
|
||
it("should allow store changes through when set to block", function() | ||
local function reducer(state, action) | ||
if state == nil then | ||
state = 0 | ||
end | ||
|
||
if action.type == "increment" then | ||
return state + 1 | ||
end | ||
|
||
return state | ||
end | ||
|
||
local store = Rodux.Store.new(reducer) | ||
|
||
local lastProps | ||
local function TestComponent(props) | ||
lastProps = props | ||
return nil | ||
end | ||
|
||
TestComponent = connect( | ||
function(state) | ||
return { | ||
state = state, | ||
} | ||
end | ||
)(TestComponent) | ||
|
||
local function tree(shouldBlock) | ||
return Roact.createElement(StoreProvider, { | ||
store = store, | ||
}, { | ||
Blocker = Roact.createElement(StoreUpdateGate, { | ||
shouldBlockUpdates = shouldBlock, | ||
}, { | ||
Child = Roact.createElement(TestComponent), | ||
}), | ||
}) | ||
end | ||
|
||
local handle = Roact.mount(tree(false)) | ||
|
||
expect(lastProps).to.be.ok() | ||
expect(lastProps.state).to.equal(0) | ||
|
||
store:dispatch({ type = "increment" }) | ||
store:flush() | ||
|
||
expect(lastProps.state).to.equal(1) | ||
|
||
handle = Roact.reconcile(handle, tree(true)) | ||
|
||
store:dispatch({ type = "increment" }) | ||
store:flush() | ||
|
||
expect(lastProps.state).to.equal(1) | ||
|
||
handle = Roact.reconcile(handle, tree(false)) | ||
|
||
expect(lastProps.state).to.equal(2) | ||
|
||
store:dispatch({ type = "increment" }) | ||
store:flush() | ||
|
||
expect(lastProps.state).to.equal(3) | ||
|
||
Roact.unmount(handle) | ||
end) | ||
end |
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.
Shouldn't this check if the oldState ~= newState before firing? Does Rodux itself do this?