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 EventListener fbjs utility #11617

Closed
wants to merge 1 commit into from
Closed

Conversation

nhunzaker
Copy link
Contributor

EventListener normalizes event subscription for <= IE8. This is no longer necessary. element.addEventListener is sufficient.

Follow-up from #11515

EventListener normalizes event subscription for <= IE8. This is no
longer necessary. element.addEventListener is sufficient.
@@ -124,10 +123,10 @@ export function trapBubbledEvent(topLevelType, handlerBaseName, element) {
if (!element) {
return null;
}
return EventListener.listen(
element,
element.addEventListener(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is only equivalent if element always has addEventListener. The original code in fbjs checked for it.

Are we 100% certain it always exists?

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.

Turns out I tried this before: #10592 (review). We couldn't do it because we rely on fbjs/lib/EventListener hook being overridden for Facebook.com (we have some custom logic there). I don't think we can easily drop this without coming up with some other solution for us.

@gaearon
Copy link
Collaborator

gaearon commented Dec 7, 2017

I have an alternative: #11797

@gaearon
Copy link
Collaborator

gaearon commented Dec 7, 2017

Superseded by #11797.

@gaearon gaearon closed this Dec 7, 2017
@gaearon gaearon deleted the remove-event-listener branch December 7, 2017 22:31
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.

3 participants