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

Don't use EventListener Fork in Modern WWW Builds #18333

Merged
merged 2 commits into from Mar 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 17 additions & 4 deletions packages/react-dom/src/events/EventListener.js
Expand Up @@ -11,37 +11,50 @@ export function addEventBubbleListener(
target: EventTarget,
eventType: string,
listener: Function,
): void {
): Function {
target.addEventListener(eventType, listener, false);
return listener;
}

export function addEventCaptureListener(
target: EventTarget,
eventType: string,
listener: Function,
): void {
): Function {
target.addEventListener(eventType, listener, true);
return listener;
}

export function addEventCaptureListenerWithPassiveFlag(
target: EventTarget,
eventType: string,
listener: Function,
passive: boolean,
): void {
): Function {
target.addEventListener(eventType, listener, {
capture: true,
passive,
});
return listener;
}

export function addEventBubbleListenerWithPassiveFlag(
target: EventTarget,
eventType: string,
listener: Function,
passive: boolean,
): void {
): Function {
target.addEventListener(eventType, listener, {
passive,
});
return listener;
}

export function removeEventListener(
target: EventTarget,
eventType: string,
listener: Function,
capture: boolean,
): void {
target.removeEventListener(eventType, listener, capture);
}
56 changes: 18 additions & 38 deletions packages/react-dom/src/events/ReactDOMEventListener.js
Expand Up @@ -49,6 +49,7 @@ import {
addEventCaptureListener,
addEventCaptureListenerWithPassiveFlag,
addEventBubbleListenerWithPassiveFlag,
removeEventListener,
} from './EventListener';
import getEventTarget from './getEventTarget';
import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree';
Expand Down Expand Up @@ -108,7 +109,6 @@ export function addResponderEventSystemEvent(
} else {
eventFlags |= IS_ACTIVE;
}
let fbListener;
// Check if interactive and wrap in discreteUpdates
const listener = dispatchEvent.bind(
null,
Expand All @@ -117,19 +117,15 @@ export function addResponderEventSystemEvent(
document,
);
if (passiveBrowserEventsSupported) {
fbListener = addEventCaptureListenerWithPassiveFlag(
return addEventCaptureListenerWithPassiveFlag(
document,
topLevelType,
listener,
passive,
);
} else {
fbListener = addEventCaptureListener(document, topLevelType, listener);
return addEventCaptureListener(document, topLevelType, listener);
}
// If we have an fbListener, then use that.
// We'll only have one if we use the forked
// EventListener-www module in FB builds.
return fbListener || listener;
}

export function addTrappedEventListener(
Expand Down Expand Up @@ -184,8 +180,8 @@ export function addTrappedEventListener(
const validTargetContainer = ((targetContainer: any): EventTarget);

const rawEventName = getRawEventName(topLevelType);
let fbListener;

let unsubscribeListener;
// When legacyFBSupport is enabled, it's for when we
// want to add a one time event listener to a container.
// This should only be used with enableLegacyFBPrimerSupport
Expand All @@ -203,28 +199,26 @@ export function addTrappedEventListener(
try {
return originalListener.apply(this, p);
} finally {
if (fbListener) {
fbListener.remove();
} else {
validTargetContainer.removeEventListener(
((rawEventName: any): string),
(listener: any),
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This wasn't specifying the capture flag but I fixed that.

}
removeEventListener(
validTargetContainer,
rawEventName,
unsubscribeListener,
capture,
);
}
};
}
if (capture) {
if (enableUseEventAPI && passive !== undefined) {
// This is only used with passive is either true or false.
fbListener = addEventCaptureListenerWithPassiveFlag(
unsubscribeListener = addEventCaptureListenerWithPassiveFlag(
validTargetContainer,
rawEventName,
listener,
passive,
);
} else {
fbListener = addEventCaptureListener(
unsubscribeListener = addEventCaptureListener(
validTargetContainer,
rawEventName,
listener,
Expand All @@ -233,24 +227,21 @@ export function addTrappedEventListener(
} else {
if (enableUseEventAPI && passive !== undefined) {
// This is only used with passive is either true or false.
fbListener = addEventBubbleListenerWithPassiveFlag(
unsubscribeListener = addEventBubbleListenerWithPassiveFlag(
validTargetContainer,
rawEventName,
listener,
passive,
);
} else {
fbListener = addEventBubbleListener(
unsubscribeListener = addEventBubbleListener(
validTargetContainer,
rawEventName,
listener,
);
}
}
// If we have an fbListener, then use that.
// We'll only have one if we use the forked
// EventListener-www module in FB builds.
return fbListener || listener;
return unsubscribeListener;
}

export function removeTrappedEventListener(
Expand All @@ -259,20 +250,9 @@ export function removeTrappedEventListener(
capture: boolean,
listener: any => void,
passive: void | boolean,
Copy link
Contributor

@trueadm trueadm Mar 18, 2020

Choose a reason for hiding this comment

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

This argument is not used anymore, I'll address in #18336.

) {
if (listener.remove != null) {
listener.remove();
} else {
const rawEventName = getRawEventName(topLevelType);
if (passiveBrowserEventsSupported) {
targetContainer.removeEventListener(rawEventName, listener, {
capture,
passive,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The passive flag is not needed when removing. Only the capture flag needs to be specified.

});
} else {
targetContainer.removeEventListener(rawEventName, listener, capture);
}
}
): void {
const rawEventName = getRawEventName(topLevelType);
removeEventListener(targetContainer, rawEventName, listener, capture);
}

function dispatchDiscreteEvent(
Expand Down
9 changes: 9 additions & 0 deletions packages/react-dom/src/events/forks/EventListener-www.js
Expand Up @@ -56,6 +56,15 @@ export function addEventBubbleListenerWithPassiveFlag(
);
}

export function removeEventListener(
target: EventTarget,
eventType: string,
listener: Function,
capture: boolean,
) {
listener.remove();
}

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
type Check<_X, Y: _X, X: Y = _X> = null;
Expand Down
18 changes: 16 additions & 2 deletions scripts/rollup/forks.js
Expand Up @@ -19,6 +19,15 @@ const {
} = bundleTypes;
const {RENDERER, RECONCILER} = moduleTypes;

const RELEASE_CHANNEL = process.env.RELEASE_CHANNEL;

// Default to building in experimental mode. If the release channel is set via
// an environment variable, then check if it's "experimental".
const __EXPERIMENTAL__ =
typeof RELEASE_CHANNEL === 'string'
? RELEASE_CHANNEL === 'experimental'
: true;

// If you need to replace a file with another file for a specific environment,
// add it to this list with the logic for choosing the right replacement.
const forks = Object.freeze({
Expand Down Expand Up @@ -442,8 +451,13 @@ const forks = Object.freeze({
case FB_WWW_DEV:
case FB_WWW_PROD:
case FB_WWW_PROFILING:
// Use the www fork which is integrated with TimeSlice profiling.
return 'react-dom/src/events/forks/EventListener-www.js';
if (__EXPERIMENTAL__) {
// In modern builds we don't use the indirection. We just use raw DOM.
return null;
} else {
// Use the www fork which is integrated with TimeSlice profiling.
return 'react-dom/src/events/forks/EventListener-www.js';
}
default:
return null;
}
Expand Down