Skip to content

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

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.

Sign up for GitHub

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

Closed
gaearon wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90cd545
Remove EventListener dependency from test
gaearon Sep 1, 2017
4592f72
Remove EventListener from the build
gaearon Sep 1, 2017
984cf7f
Inline event removal code into Stack version
gaearon Sep 1, 2017
5c79c32
Remove fbjs/lib/focusNode dependency
gaearon Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/renderers/dom/shared/ReactBrowserEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,15 +225,15 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
},

trapBubbledEvent: function(topLevelType, handlerBaseName, handle) {
return ReactDOMEventListener.trapBubbledEvent(
ReactDOMEventListener.trapBubbledEvent(
Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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

topLevelType,
handlerBaseName,
handle,
);
},

trapCapturedEvent: function(topLevelType, handlerBaseName, handle) {
return ReactDOMEventListener.trapCapturedEvent(
ReactDOMEventListener.trapCapturedEvent(
topLevelType,
handlerBaseName,
handle,
Expand Down
13 changes: 6 additions & 7 deletions src/renderers/dom/shared/ReactDOMEventListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

'use strict';

var EventListener = require('fbjs/lib/EventListener');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactFiberTreeReflection = require('ReactFiberTreeReflection');
var ReactGenericBatching = require('ReactGenericBatching');
Expand Down Expand Up @@ -138,12 +137,12 @@ var ReactDOMEventListener = {
*/
trapBubbledEvent: function(topLevelType, handlerBaseName, element) {
if (!element) {
return null;
return;
}
return EventListener.listen(
element,
element.addEventListener(
Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@aweary aweary Sep 1, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sophiebits sophiebits Sep 1, 2017

Choose a reason for hiding this comment

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

I think EventListener gets shimmed in www?

Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nhunzaker nhunzaker Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@sophiebits sophiebits Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@aweary aweary Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

@aweary Yes I think we can.

handlerBaseName,
ReactDOMEventListener.dispatchEvent.bind(null, topLevelType),
false,
);
},

Expand All @@ -159,12 +158,12 @@ var ReactDOMEventListener = {
*/
trapCapturedEvent: function(topLevelType, handlerBaseName, element) {
if (!element) {
return null;
return;
}
return EventListener.capture(
element,
element.addEventListener(
handlerBaseName,
ReactDOMEventListener.dispatchEvent.bind(null, topLevelType),
true,
);
},

Expand Down
3 changes: 1 addition & 2 deletions src/renderers/dom/shared/ReactInputSelection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ var ReactDOMSelection = require('ReactDOMSelection');
var {ELEMENT_NODE} = require('HTMLNodeType');

var containsNode = require('fbjs/lib/containsNode');
var focusNode = require('fbjs/lib/focusNode');
var getActiveElement = require('fbjs/lib/getActiveElement');

function isInDocument(node) {
Expand Down Expand Up @@ -76,7 +75,7 @@ var ReactInputSelection = {
}
}

focusNode(priorFocusedElem);
priorFocusedElem.focus();

for (let i = 0; i < ancestors.length; i++) {
const info = ancestors[i];
Expand Down
31 changes: 8 additions & 23 deletions src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

'use strict';

var EventListener;
var EventPluginHub;
var EventPluginRegistry;
var React;
Expand Down Expand Up @@ -59,7 +58,6 @@ describe('ReactBrowserEventEmitter', () => {
beforeEach(() => {
jest.resetModules();
LISTENER.mockClear();
EventListener = require('fbjs/lib/EventListener');
// TODO: can we express this test with only public API?
EventPluginHub = require('EventPluginHub');
EventPluginRegistry = require('EventPluginRegistry');
Expand Down Expand Up @@ -376,43 +374,30 @@ describe('ReactBrowserEventEmitter', () => {
});

it('should listen to events only once', () => {
spyOn(EventListener, 'listen');
spyOn(document, 'addEventListener');
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document);
ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document);
expect(EventListener.listen.calls.count()).toBe(1);
expect(document.addEventListener.calls.count()).toBe(1);
});

it('should work with event plugins without dependencies', () => {
spyOn(EventListener, 'listen');
spyOn(document, 'addEventListener');

ReactBrowserEventEmitter.listenTo(ON_CLICK_KEY, document);

expect(EventListener.listen.calls.argsFor(0)[1]).toBe('click');
expect(document.addEventListener.calls.argsFor(0)[0]).toBe('click');
});

it('should work with event plugins with dependencies', () => {
spyOn(EventListener, 'listen');
spyOn(EventListener, 'capture');
spyOn(document, 'addEventListener');

ReactBrowserEventEmitter.listenTo(ON_CHANGE_KEY, document);

var setEventListeners = [];
var listenCalls = EventListener.listen.calls.allArgs();
var captureCalls = EventListener.capture.calls.allArgs();
for (var i = 0; i < listenCalls.length; i++) {
setEventListeners.push(listenCalls[i][1]);
}
for (i = 0; i < captureCalls.length; i++) {
setEventListeners.push(captureCalls[i][1]);
}
Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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


var module = EventPluginRegistry.registrationNameModules[ON_CHANGE_KEY];
var dependencies = module.eventTypes.change.dependencies;
expect(setEventListeners.length).toEqual(dependencies.length);

for (i = 0; i < setEventListeners.length; i++) {
expect(dependencies.indexOf(setEventListeners[i])).toBeTruthy();
Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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.

}
expect(document.addEventListener.calls.count()).toEqual(
dependencies.length,
);
});

it('should bubble onTouchTap', () => {
Expand Down
4 changes: 1 addition & 3 deletions src/renderers/dom/shared/utils/AutoFocusUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@

var ReactDOMComponentTree = require('ReactDOMComponentTree');

var focusNode = require('fbjs/lib/focusNode');

var AutoFocusUtils = {
focusDOMComponent: function() {
focusNode(ReactDOMComponentTree.getNodeFromInstance(this));
ReactDOMComponentTree.getNodeFromInstance(this).focus();
Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

This is Stack-only codepath.

},
};

Expand Down
40 changes: 23 additions & 17 deletions src/renderers/dom/stack/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var EventPluginRegistry = require('EventPluginRegistry');
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
var ReactDOMComponentFlags = require('ReactDOMComponentFlags');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactDOMEventListener = require('ReactDOMEventListener');
var ReactDOMInput = require('ReactDOMInput');
var ReactDOMOption = require('ReactDOMOption');
var ReactDOMSelect = require('ReactDOMSelect');
Expand Down Expand Up @@ -276,11 +277,24 @@ function trapBubbledEventsLocal() {
var node = getNode(inst);
invariant(node, 'trapBubbledEvent(...): Requires node to be rendered.');

function trapBubbledEvent(topLevelType, handlerBaseName, element) {
if (!element) {
return;
}
var callback = ReactDOMEventListener.dispatchEvent.bind(null, topLevelType);
element.addEventListener(handlerBaseName, callback, false);
return {
remove() {
element.removeEventListener(handlerBaseName, callback, false);
},
};
}

switch (inst._tag) {
case 'iframe':
case 'object':
inst._wrapperState.listeners = [
ReactBrowserEventEmitter.trapBubbledEvent('topLoad', 'load', node),
trapBubbledEvent('topLoad', 'load', node),
Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@aweary aweary Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@gaearon gaearon Sep 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nhunzaker nhunzaker Sep 1, 2017

Choose a reason for hiding this comment

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

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.

];
break;
case 'video':
Expand All @@ -290,47 +304,39 @@ function trapBubbledEventsLocal() {
for (var event in mediaEvents) {
if (mediaEvents.hasOwnProperty(event)) {
inst._wrapperState.listeners.push(
ReactBrowserEventEmitter.trapBubbledEvent(
event,
mediaEvents[event],
node,
),
trapBubbledEvent(event, mediaEvents[event], node),
);
}
}
break;
case 'source':
inst._wrapperState.listeners = [
ReactBrowserEventEmitter.trapBubbledEvent('topError', 'error', node),
trapBubbledEvent('topError', 'error', node),
];
break;
case 'img':
case 'image':
inst._wrapperState.listeners = [
ReactBrowserEventEmitter.trapBubbledEvent('topError', 'error', node),
ReactBrowserEventEmitter.trapBubbledEvent('topLoad', 'load', node),
trapBubbledEvent('topError', 'error', node),
trapBubbledEvent('topLoad', 'load', node),
];
break;
case 'form':
inst._wrapperState.listeners = [
ReactBrowserEventEmitter.trapBubbledEvent('topReset', 'reset', node),
ReactBrowserEventEmitter.trapBubbledEvent('topSubmit', 'submit', node),
trapBubbledEvent('topReset', 'reset', node),
trapBubbledEvent('topSubmit', 'submit', node),
];
break;
case 'input':
case 'select':
case 'textarea':
inst._wrapperState.listeners = [
ReactBrowserEventEmitter.trapBubbledEvent(
'topInvalid',
'invalid',
node,
),
trapBubbledEvent('topInvalid', 'invalid', node),
];
break;
case 'details':
inst._wrapperState.listeners = [
ReactBrowserEventEmitter.trapBubbledEvent('topToggle', 'toggle', node),
trapBubbledEvent('topToggle', 'toggle', node),
];
break;
}
Expand Down