Remove the use of proxies for synthetic events in DEV #13225
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
This was a bug introduced by facebook#5947. It's very confusing that they become nulled while stopPropagation/preventDefault don't.
|
|
|
ReactDOM: size: -0.0%, gzip: -0.1% Details of bundled changes.Comparing: 171e0b7...2f3a2d0 react-dom
react-native-renderer
Generated by 🚫 dangerJS |
bvaughn
approved these changes
Jul 17, 2018
| @@ -270,25 +326,25 @@ describe('SyntheticEvent', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('should warn if Proxy is supported and the synthetic event is added a property', () => { | |||
| // TODO: we might want to re-add a warning like this in the future, | |||
| // but it shouldn't use Proxies because they making debugging difficult. | |||
| // but it shouldn't use Proxies because they making debugging difficult. | ||
| // Or we might disallow this pattern altogether: | ||
| // https://github.com/facebook/react/issues/13224 | ||
| xit('should warn if synthetic event is added a property', () => { |
There was a problem hiding this comment.
supernit "should warn if a property is added to a synthetic event"
There was a problem hiding this comment.
Yeah, it already had the grammar issue before. Will do
This was referenced Jul 17, 2018
|
Late to the game on this, but this is a great change! 👍 |
|
We should've done this like a year earlier 😛 |
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.
Fixes #12171.
Motivation: this is very confusing. I've seen quite a few complaints about this and also bumped into it myself when I played with React occasionally. The warning is just not worth it.
The first commit (ac71c29) is pretty much a revert of #5947. #5947 cleared a few more properties than the code before it (which is nice) so I kept the current behavior (but removed the key list indirection).
The second commit (3c97680) fixes a bug that was introduced by #5947. Specifically, it was nulling out
isPropagationStopped()andisDefaultPrevented(). While you shouldn't access them, nulling them out can lead to confusing crashes. So I just changed them to behave similarly topreventDefault()andstopPropagation()(which get shimmed with noops while the event is pooled). I consider it a bugfix — can't imagine somebody relying on whether it's nulled out or not in a way that isn't broken for some other reason.I considered adding some kind of warning instead of this one (e.g. by counting the keys on every release). But it just seemed too complicated and not worth it. I imagine the situation is pretty rare anyway. And we can fix it in 17 for good with #13224.