-
Notifications
You must be signed in to change notification settings - Fork 143
LUAFDN-268 optimize createSignal memory usage #304
Changes from 7 commits
e4b0286
bc048fe
808a2de
5059abf
5a1d024
7c61e19
ad57c97
0f8300b
edcec4e
9ad304e
725b4f5
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 |
---|---|---|
|
@@ -86,4 +86,69 @@ return function() | |
-- Exactly once listener should have been called. | ||
expect(spyA.callCount + spyB.callCount).to.equal(1) | ||
end) | ||
|
||
it("should allow adding listener in the middle of firing", function() | ||
local signal = createSignal() | ||
|
||
local disconnectA | ||
local spyA = createSpy() | ||
local listener = function(a, b) | ||
disconnectA = signal:subscribe(spyA.value) | ||
end | ||
|
||
local disconnectListener = signal:subscribe(listener) | ||
|
||
expect(spyA.callCount).to.equal(0) | ||
|
||
local a = {} | ||
local b = 67 | ||
signal:fire(a, b) | ||
|
||
expect(spyA.callCount).to.equal(0) | ||
|
||
-- The new listener should be picked up in next fire. | ||
signal:fire(b, a) | ||
expect(spyA.callCount).to.equal(1) | ||
spyA:assertCalledWith(b, a) | ||
|
||
disconnectA() | ||
disconnectListener() | ||
|
||
signal:fire(a) | ||
|
||
expect(spyA.callCount).to.equal(1) | ||
end) | ||
|
||
it("should have one connection instance when add the same listener multiple times", function() | ||
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 have it warn the user when adding the same one multiple times? when this happens, it's probably the symptom of a deeper bug, right? 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. For what it's worth, we try to be cautious about warnings when Roact powers things like in-game menu and studio plugins. React doesn't actually expose its signal implementation, so warning about this would signal an internal bug. If this cropped up in shipped code, we'd end up confusingly printing warnings to developers in studio about things that have nothing to do with them! 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. I've added a warning message. 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. Yeah I'm a bit worried about the warning too, I am not sure if that can bubble up to the consumers of Roact. I've checked out createContext and Binding and it seems ok to me 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. I will remove it for now. We can loop back on the warning message in a separate PR if necessary |
||
local signal = createSignal() | ||
|
||
local spyA = createSpy() | ||
local disconnect1 = signal:subscribe(spyA.value) | ||
|
||
expect(spyA.callCount).to.equal(0) | ||
|
||
local a = {} | ||
local b = 67 | ||
signal:fire(a, b) | ||
|
||
expect(spyA.callCount).to.equal(1) | ||
spyA:assertCalledWith(a, b) | ||
|
||
local disconnect2 = signal:subscribe(spyA.value) | ||
|
||
signal:fire(b, a) | ||
expect(spyA.callCount).to.equal(2) | ||
spyA:assertCalledWith(b, a) | ||
|
||
disconnect2() | ||
|
||
signal:fire(a) | ||
|
||
expect(spyA.callCount).to.equal(2) | ||
|
||
-- should have no effect. | ||
disconnect1() | ||
signal:fire(a) | ||
expect(spyA.callCount).to.equal(2) | ||
end) | ||
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 you add a link to the PR like the other items in the list?
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.
Done.