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 143
LUAFDN-268 optimize createSignal memory usage #304
Merged
oltrep
merged 11 commits into
Roblox:master
from
yjia2:feature/LUAFDN-268-optimize-createSignal-memory-usage
May 13, 2021
Merged
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e4b0286
optimize the createSignal implementation to consume less memory
yjia2 bc048fe
update change log
yjia2 808a2de
fix the test failure
yjia2 5059abf
remove debug log
yjia2 5a1d024
add yjia2 to the CLA whitelist
yjia2 7c61e19
revert workflow changes
yjia2 ad57c97
add test to cover multiple registering same listener
yjia2 0f8300b
Merge branch 'master' into feature/LUAFDN-268-optimize-createSignal-m…
yjia2 edcec4e
address comments
yjia2 9ad304e
Merge branch 'feature/LUAFDN-268-optimize-createSignal-memory-usage' …
yjia2 725b4f5
remove warning messages
yjia2 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
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.
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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