Warn if MutableSource snapshot is a function #18933
Merged
Conversation
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
facebook-github-bot
added
CLA Signed
React Core Team
bvaughn
commented
May 15, 2020
| if (typeof snapshot === 'function') { | ||
| console.warn( | ||
| 'Mutable source should not return a functions as the snapshot value. ' + | ||
| 'Functions may close over mutable values and cause tearing.', |
There was a problem hiding this comment.
This message probably isn't great. Happy to change it.
bvaughn
force-pushed
the
useMutableSource-function-value-warning
branch
from
6f52afb
to
d45cd7d
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6f52afb:
|
Details of bundled changes.Comparing: 142d4f1...2c45e61 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Size changes (experimental) |
Details of bundled changes.Comparing: 142d4f1...2c45e61 react-native-renderer
react-dom
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2c45e61:
|
sebmarkbage
reviewed
May 21, 2020
| const snapshot = getSnapshot(source._source); | ||
| if (__DEV__) { | ||
| if (typeof snapshot === 'function') { | ||
| console.warn( |
There was a problem hiding this comment.
Should it be console.error? Seems very error like if it might give wrong output and we might make it an invariant.
sebmarkbage
approved these changes
May 21, 2020
added 2 commits
May 21, 2020 16:01
useMutableSource does not properly support snapshots that are functions. In part this is because of how it is implemented internally (the function gets mistaken for a state updater function). To fix this we could just wrap another function around the returned snapshot, but this pattern seems problematic to begin with- because the function that gets returned might itself close over mutable values, which would defeat the purpose of using the hook in the first place.
This PR proposes adding a new DEV warning if the snapshot returned is a function. It does not change the behavior (meaning that a function could still work in some cases- but at least the current behavior prevents passing around a closure that may later become stale unless you're really intentional about it e.g. () => () => {...}).
bvaughn
force-pushed
the
useMutableSource-function-value-warning
branch
from
d45cd7d
to
2c45e61
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #18823; potential alternative of #18921.
useMutableSourcedoes not properly support snapshots that are functions. In part this is because of how it is implemented internally (the function gets mistaken for a state updater function). To "fix" this we could just wrap another function around the returned snapshot, but this pattern seems problematic to begin with- because the function that gets returned might itself close over mutable values, which would defeat the purpose of using the hook in the first place.This PR proposes adding a new DEV warning if the snapshot returned is a function. It does not change the behavior (meaning that a function could still "work" in some cases- but at least the current behavior prevents passing around a closure that may later become stale unless you're really intentional about it e.g.
() => () => {...}).Open to discussion on whether we should lock it down further and treat this as an invariant.