From 04ef7b8a2401b22718c7dbd89ed48d6475e1e952 Mon Sep 17 00:00:00 2001 From: oltrep Date: Thu, 16 Sep 2021 14:45:32 -0700 Subject: [PATCH 1/4] add test that demonstrate the bug --- src/createContext.spec.lua | 171 +++++++++++++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/src/createContext.spec.lua b/src/createContext.spec.lua index 432d39d4..09d7b128 100644 --- a/src/createContext.spec.lua +++ b/src/createContext.spec.lua @@ -1,4 +1,6 @@ return function() + local ReplicatedStorage = game:GetService("ReplicatedStorage") + local Component = require(script.Parent.Component) local NoopRenderer = require(script.Parent.NoopRenderer) local Children = require(script.Parent.PropMarkers.Children) @@ -10,6 +12,9 @@ return function() local noopReconciler = createReconciler(NoopRenderer) + local RobloxRenderer = require(script.Parent.RobloxRenderer) + local robloxReconciler = createReconciler(RobloxRenderer) + it("should return a table", function() local context = createContext("Test") expect(context).to.be.ok() @@ -301,4 +306,170 @@ return function() expect(observedB).to.equal(true) end) end) + + -- issue https://github.com/Roblox/roact/issues/319 + it("does not throw if willUnmount is called twice on a context consumer", function() + local context = createContext({}) + + local unmountCounts = {} + local didMountCounts = {} + + local function addUnmount(component) + unmountCounts[component] = unmountCounts[component] + 1 + end + local function addDidMount(component) + didMountCounts[component] = didMountCounts[component] + 1 + end + + local elementCounter = 0 + local function addInit(component) + elementCounter = elementCounter + 1 + component._id = ("%s (%s)"):format(component.__componentName, elementCounter) + unmountCounts[component] = 0 + didMountCounts[component] = 0 + end + + local function reportCounters() + do + local data = {} + for component, count in pairs(didMountCounts) do + table.insert(data, ("\n\t%s: %d (%s)"):format(component._id, count, tostring(component._wasMounted))) + end + table.sort(data) + local dataOutput = table.concat(data, "") + + warn(("Mount -> {%s\n}"):format(dataOutput)) + end + do + local data = {} + for component, count in pairs(unmountCounts) do + table.insert(data, ("\n\t%s: %d (%s)"):format(component._id, count, tostring(component._wasMounted))) + end + table.sort(data) + local dataOutput = table.concat(data, "") + + warn(("Unmount -> {%s\n}"):format(dataOutput)) + end + end + + local LowestComponent = Component:extend("LowestComponent") + function LowestComponent:init() + addInit(self) + end + + function LowestComponent:render() + return createElement("Frame") + end + + function LowestComponent:didMount() + addDidMount(self) + self._wasMounted = true + -- print("DID MOUNT LowestComponent: ", self._id) + self.props.onDidMountCallback() + end + + function LowestComponent:willUnmount() + addUnmount(self) + end + + local FirstComponent = Component:extend("FirstComponent") + function FirstComponent:init() + addInit(self) + end + + function FirstComponent:render() + return createElement(context.Consumer, { + render = function() + return createElement("TextLabel") + end, + }) + end + + function FirstComponent:didMount() + addDidMount(self) + self._wasMounted = true + end + + function FirstComponent:willUnmount() + addUnmount(self) + end + + local ChildComponent = Component:extend("ChildComponent") + + function ChildComponent:init() + addInit(self) + self:setState({ firstTime = true }) + end + + local childCallback + + function ChildComponent:render() + if self.state.firstTime then + return createElement(FirstComponent) + end + + return createElement(LowestComponent, { + onDidMountCallback = self.props.onDidMountCallback + }) + end + + function ChildComponent:didMount() + addDidMount(self) + self._wasMounted = true + childCallback = function() + self:setState({ firstTime = false }) + end + end + + function ChildComponent:willUnmount() + addUnmount(self) + end + + local ParentComponent = Component:extend("ParentComponent") + + local didMountCallbackCalled = 0 + + function ParentComponent:init() + self:setState({ count = 1 }) + + self.onDidMountCallback = function() + didMountCallbackCalled = didMountCallbackCalled + 1 + if self.state.count < 5 then + self:setState({ count = self.state.count + 1 }) + end + end + end + + function ParentComponent:render() + return createElement("Frame", {}, { + Provider = createElement(context.Provider, { + value = {}, + }, { + ChildComponent = createElement(ChildComponent, { + count = self.state.count, + onDidMountCallback = self.onDidMountCallback, + }), + }) + }) + end + + local parent = Instance.new("ScreenGui") + parent.Parent = ReplicatedStorage + + local hostKey = "Some Key" + local node = robloxReconciler.mountVirtualNode(createElement(ParentComponent), parent, hostKey) + + reportCounters() + + expect(function() + -- calling setState on ChildComponent will trigger `willUnmount` multiple times + childCallback() + end).never.to.throw() + + reportCounters() + + expect(function() + robloxReconciler.unmountVirtualNode(node) + end).never.to.throw() + end) end \ No newline at end of file From a60dda15a10afa7bb64a973ba5681732b47e094f Mon Sep 17 00:00:00 2001 From: oltrep Date: Thu, 16 Sep 2021 14:48:02 -0700 Subject: [PATCH 2/4] clean up debug output from test --- src/createContext.spec.lua | 76 +------------------------------------- 1 file changed, 1 insertion(+), 75 deletions(-) diff --git a/src/createContext.spec.lua b/src/createContext.spec.lua index 09d7b128..4b436b86 100644 --- a/src/createContext.spec.lua +++ b/src/createContext.spec.lua @@ -311,50 +311,8 @@ return function() it("does not throw if willUnmount is called twice on a context consumer", function() local context = createContext({}) - local unmountCounts = {} - local didMountCounts = {} - - local function addUnmount(component) - unmountCounts[component] = unmountCounts[component] + 1 - end - local function addDidMount(component) - didMountCounts[component] = didMountCounts[component] + 1 - end - - local elementCounter = 0 - local function addInit(component) - elementCounter = elementCounter + 1 - component._id = ("%s (%s)"):format(component.__componentName, elementCounter) - unmountCounts[component] = 0 - didMountCounts[component] = 0 - end - - local function reportCounters() - do - local data = {} - for component, count in pairs(didMountCounts) do - table.insert(data, ("\n\t%s: %d (%s)"):format(component._id, count, tostring(component._wasMounted))) - end - table.sort(data) - local dataOutput = table.concat(data, "") - - warn(("Mount -> {%s\n}"):format(dataOutput)) - end - do - local data = {} - for component, count in pairs(unmountCounts) do - table.insert(data, ("\n\t%s: %d (%s)"):format(component._id, count, tostring(component._wasMounted))) - end - table.sort(data) - local dataOutput = table.concat(data, "") - - warn(("Unmount -> {%s\n}"):format(dataOutput)) - end - end - local LowestComponent = Component:extend("LowestComponent") function LowestComponent:init() - addInit(self) end function LowestComponent:render() @@ -362,19 +320,11 @@ return function() end function LowestComponent:didMount() - addDidMount(self) - self._wasMounted = true - -- print("DID MOUNT LowestComponent: ", self._id) self.props.onDidMountCallback() end - function LowestComponent:willUnmount() - addUnmount(self) - end - local FirstComponent = Component:extend("FirstComponent") function FirstComponent:init() - addInit(self) end function FirstComponent:render() @@ -385,19 +335,9 @@ return function() }) end - function FirstComponent:didMount() - addDidMount(self) - self._wasMounted = true - end - - function FirstComponent:willUnmount() - addUnmount(self) - end - local ChildComponent = Component:extend("ChildComponent") function ChildComponent:init() - addInit(self) self:setState({ firstTime = true }) end @@ -414,17 +354,11 @@ return function() end function ChildComponent:didMount() - addDidMount(self) - self._wasMounted = true childCallback = function() self:setState({ firstTime = false }) end end - function ChildComponent:willUnmount() - addUnmount(self) - end - local ParentComponent = Component:extend("ParentComponent") local didMountCallbackCalled = 0 @@ -457,19 +391,11 @@ return function() parent.Parent = ReplicatedStorage local hostKey = "Some Key" - local node = robloxReconciler.mountVirtualNode(createElement(ParentComponent), parent, hostKey) - - reportCounters() + robloxReconciler.mountVirtualNode(createElement(ParentComponent), parent, hostKey) expect(function() -- calling setState on ChildComponent will trigger `willUnmount` multiple times childCallback() end).never.to.throw() - - reportCounters() - - expect(function() - robloxReconciler.unmountVirtualNode(node) - end).never.to.throw() end) end \ No newline at end of file From 8ac548f8a2d1f45b581b1001d59b23f77e4b9a60 Mon Sep 17 00:00:00 2001 From: oltrep Date: Thu, 16 Sep 2021 15:00:03 -0700 Subject: [PATCH 3/4] add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d7b70bf..844a4a2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased Changes +* Fixed `Listeners can only be disconnected once` from context consumers. ([#320](https://github.com/Roblox/roact/pull/320)) + ## [1.4.1](https://github.com/Roblox/roact/releases/tag/v1.4.1) (August 12th, 2021) * Fixed a bug where the Roact tree could get into a broken state when using callbacks passed to a child component. Updated the tempFixUpdateChildrenReEntrancy config value to also handle this case. ([#315](https://github.com/Roblox/roact/pull/315)) * Fixed forwardRef description ([#312](https://github.com/Roblox/roact/pull/312)). From ae8edf0289137b093544b4d01e5ba0f70ff24451 Mon Sep 17 00:00:00 2001 From: oltrep Date: Thu, 16 Sep 2021 15:01:17 -0700 Subject: [PATCH 4/4] fix and make the test pass --- src/createContext.lua | 1 + 1 file changed, 1 insertion(+) diff --git a/src/createContext.lua b/src/createContext.lua index b21635ef..e4dcae15 100644 --- a/src/createContext.lua +++ b/src/createContext.lua @@ -119,6 +119,7 @@ local function createConsumer(context) function Consumer:willUnmount() if self.disconnect ~= nil then self.disconnect() + self.disconnect = nil end end