Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove trapBubbledEventsLocal #10258

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

jfo84
Copy link
Contributor

@jfo84 jfo84 commented Jul 22, 2017

This PR moves calls to trapBubbledEventsLocal outside of an equivalent switch statement of html tags.

This is a duplicate of #9973. I think y'all were busy cleaning up 15.6 when I first PR'ed this.

@gaearon
Copy link
Collaborator

gaearon commented Jul 31, 2017

Hmm. I don’t think that’s what the TODO was referring to.

If I understand it right, the TODO is asking to inline the calls themselves instead of trapBubbledEventsLocal calls. So the fix would be to

  1. Completely delete trapBubbledEventsLocal
  2. Duplicate its switch branches in two respective functions calling it instead of calls to trapBubbledEventsLocal

This lets us avoid doing switch twice.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see above

@jfo84
Copy link
Contributor Author

jfo84 commented Jul 31, 2017

I took that approach in #9924 but didn't get any feedback. I just resurrected that branch and merged master and some tests are failing, but I'll take a look later. I can just revert the initial commit and squash commit onto this branch to keep the conversation in one place

@jfo84
Copy link
Contributor Author

jfo84 commented Aug 1, 2017

It looks like it was just a local issue. Ready for review @gaearon

@jfo84 jfo84 changed the title Inline trapBubbledEventsLocal Remove trapBubbledEventsLocal Aug 1, 2017
@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2017

Yea, sorry about that. This is our biggest release yet and we haven’t been reviewing things well.

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2017

Looked through the code in each case and did manual testing for some of these.
I think this is correct. Thanks!

@gaearon gaearon merged commit 04db351 into facebook:master Aug 1, 2017
@jfo84
Copy link
Contributor Author

jfo84 commented Aug 1, 2017

No worries, @gaearon. Y'all get a lot of PR's

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants