Remove dependency on fbjs/lib/EventListener and fbjs/lib/focusNode #10592

gaearon · 2017-09-01T18:12:55Z

It's pretty small but still unnecessary indirection now that we don't support IE8.
See inline comments.

Inline the part that we actually use. Drop IE8 polyfill and unsubscription code.

Stack still depends on the return value. I just inline this into Stack version since we don't really care about it.

gaearon · 2017-09-01T18:13:21Z

src/renderers/dom/shared/ReactBrowserEventEmitter.js


  trapBubbledEvent: function(topLevelType, handlerBaseName, handle) {
-    return ReactDOMEventListener.trapBubbledEvent(
+    ReactDOMEventListener.trapBubbledEvent(


Fiber codebase doesn't use return value (which is always allocating an object).

gaearon · 2017-09-01T18:14:41Z

src/renderers/dom/shared/ReactDOMEventListener.js

    }
-    return EventListener.listen(
-      element,
+    element.addEventListener(


The only case in which this wouldn't be equivalent is if element is truthy but not a DOM node. Doesn't seem like this could happen.

Would it make sense to throw if element is falsy? I don't think there's any current code path where trapBubbledEvent or trapCaptureEvent should be called without a node.

I think EventListener gets shimmed in www?

There exists a mobile version that wraps into TimeSlice guards but we don't use it. At least not since flat bundles. Nobody complained.

I'm curious about when element is null here too and would +1 an invariant or an action item to investigate this.

I don't think an invariant is very valuable here because it's a leaf code path (throws immediately with a very obvious message).

@gaearon: see https://fburl.com/8jv38orh

Aah, I get it now. We leave fbjs/lib/EventListener import in the www flat bundle, which actually redirects to the internal EventListener fork.

@gaearon what throws immediately? If this is protected from null elements earlier in the code path could we just remove the falsy check?

@aweary Yes I think we can.

gaearon · 2017-09-01T18:17:21Z

src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js

-    expect(setEventListeners.length).toEqual(dependencies.length);
-
-    for (i = 0; i < setEventListeners.length; i++) {
-      expect(dependencies.indexOf(setEventListeners[i])).toBeTruthy();


I deleted this set of assertions. I'm not sure what they're supposed to be testing but they didn't work in the first place.

The intention was probably to match whether an array contains an item, but toBeTruthy() check passes for -1. And -1 is exactly what we get on master because dependencies array looks like this:

[ 'topBlur', 'topChange', 'topClick', 'topFocus', 'topInput', 'topKeyDown', 'topKeyUp', 'topSelectionChange' ]

but value being tested looks like

'blur'

Maybe there's some better way to test this but I figured just asserting the count is enough.

gaearon · 2017-09-01T18:17:51Z

src/renderers/dom/stack/client/ReactDOMComponent.js

    case 'object':
      inst._wrapperState.listeners = [
-        ReactBrowserEventEmitter.trapBubbledEvent('topLoad', 'load', node),
+        trapBubbledEvent('topLoad', 'load', node),


Stack code still relies on return value for unsubscribing. Fiber doesn't. I inlined the old Stack-friendly version right into Stack.

How does Fiber handle removing those listeners on unmount? Does it just not do that anymore?

I think it doesn't. Relies on GC to clean them up.

At some point I need to circle back about this on #9333. We need local listeners and I'm trying to figure out the best way to clean them up.

gaearon · 2017-09-01T18:19:25Z

src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js

-    }
-    for (i = 0; i < captureCalls.length; i++) {
-      setEventListeners.push(captureCalls[i][1]);
-    }


Deleted this aggregation because it goes through the same code path now.

gaearon · 2017-09-01T18:29:35Z

src/renderers/dom/shared/utils/AutoFocusUtils.js

 var AutoFocusUtils = {
  focusDOMComponent: function() {
-    focusNode(ReactDOMComponentTree.getNodeFromInstance(this));
+    ReactDOMComponentTree.getNodeFromInstance(this).focus();


This is Stack-only codepath.

nhunzaker

Fantastic update, 👍

sophiebits

Don't think the EventListener change is right; see https://fburl.com/8jv38orh. It's important for error logging.

gaearon · 2017-09-01T20:26:39Z

Probably abandoning—seems like adding injection to work around internal hijack of EventListener would add more size than having this in the first place.

sophiebits · 2017-09-01T20:28:44Z

We can still get rid of focusNode!

gaearon added 3 commits September 1, 2017 11:01

Remove EventListener dependency from test

90cd545

Remove EventListener from the build

4592f72

Inline the part that we actually use. Drop IE8 polyfill and unsubscription code.

Inline event removal code into Stack version

984cf7f

Stack still depends on the return value. I just inline this into Stack version since we don't really care about it.

facebook-github-bot added the CLA Signed label Sep 1, 2017

gaearon commented Sep 1, 2017

View reviewed changes

gaearon requested review from aweary and sophiebits September 1, 2017 18:18

gaearon commented Sep 1, 2017

View reviewed changes

Remove fbjs/lib/focusNode dependency

5c79c32

gaearon changed the title ~~Remove dependency on fbjs/lib/EventListener~~ Remove dependency on fbjs/lib/EventListener and fbjs/lib/focusNode Sep 1, 2017

gaearon commented Sep 1, 2017

View reviewed changes

nhunzaker approved these changes Sep 1, 2017

View reviewed changes

sophiebits requested changes Sep 1, 2017

View reviewed changes

gaearon closed this Sep 1, 2017

Remove dependency on fbjs/lib/EventListener and fbjs/lib/focusNode #10592

Remove dependency on fbjs/lib/EventListener and fbjs/lib/focusNode #10592

Uh oh!

Conversation

gaearon commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhunzaker left a comment

Choose a reason for hiding this comment

Uh oh!

sophiebits left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Sep 1, 2017

Uh oh!

sophiebits commented Sep 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaearon Sep 1, 2017 •

edited

Loading

sophiebits left a comment •

edited

Loading