Skip to content

[WIP] Remove event whitelist #11992

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
jquense wants to merge 1 commit into from
Closed

[WIP] Remove event whitelist #11992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 9 additions & 2 deletions packages/events/EventPluginRegistry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ function recomputePluginOrdering(): void {
);
plugins[pluginIndex] = pluginModule;
const publishedEvents = pluginModule.eventTypes;
if (!publishedEvents) {
return;
}

for (const eventName in publishedEvents) {
invariant(
publishEventForPlugin(
Expand Down Expand Up @@ -145,8 +149,11 @@ function publishRegistrationName(
registrationName,
);
registrationNameModules[registrationName] = pluginModule;
registrationNameDependencies[registrationName] =
pluginModule.eventTypes[eventName].dependencies;

if (pluginModule.eventTypes) {
registrationNameDependencies[registrationName] =
pluginModule.eventTypes[eventName].dependencies;
}

if (__DEV__) {
const lowerCasedName = registrationName.toLowerCase();
Expand Down
2 changes: 1 addition & 1 deletion packages/events/PluginModuleType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export type AnyNativeEvent = Event | KeyboardEvent | MouseEvent | Touch;
export type PluginName = string;

export type PluginModule<NativeEvent> = {
eventTypes: EventTypes,
eventTypes?: EventTypes,
extractEvents: (
topLevelType: string,
targetInst: Fiber,
Expand Down
9 changes: 4 additions & 5 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

// TODO: direct imports like some-package/src/* are bad. Fix me.
import ReactDebugCurrentFiber from 'react-reconciler/src/ReactDebugCurrentFiber';
import {registrationNameModules} from 'events/EventPluginRegistry';
import emptyFunction from 'fbjs/lib/emptyFunction';
import warning from 'fbjs/lib/warning';

Expand Down Expand Up @@ -312,7 +311,7 @@ function setInitialDOMProperties(
} else if (propKey === AUTOFOCUS) {
// We polyfill it separately on the client during commit.
// We blacklist it here rather than in the property list because we emit it in SSR.
} else if (registrationNameModules.hasOwnProperty(propKey)) {
} else if (propKey[0] === 'o' && propKey[1] === 'n') {
Copy link
Contributor

@aweary aweary Jan 12, 2018

Choose a reason for hiding this comment

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

It might be worth extracting this into a function like isEventName so it minifies better.

if (nextProp != null) {
if (__DEV__ && typeof nextProp !== 'function') {
warnForInvalidEventListener(propKey, nextProp);
Expand Down Expand Up @@ -651,7 +650,7 @@ export function diffProperties(
// Noop
} else if (propKey === AUTOFOCUS) {
// Noop. It doesn't work on updates anyway.
} else if (registrationNameModules.hasOwnProperty(propKey)) {
} else if (propKey[0] === 'o' && propKey[1] === 'n') {
// This is a special case. If any listener updates we need to ensure
// that the "current" fiber pointer gets updated so we need a commit
// to update this element.
Expand Down Expand Up @@ -740,7 +739,7 @@ export function diffProperties(
propKey === SUPPRESS_HYDRATION_WARNING
) {
// Noop
} else if (registrationNameModules.hasOwnProperty(propKey)) {
} else if (propKey[0] === 'o' && propKey[1] === 'n') {
if (nextProp != null) {
// We eagerly listen to this even though we haven't committed yet.
if (__DEV__ && typeof nextProp !== 'function') {
Expand Down Expand Up @@ -966,7 +965,7 @@ export function diffHydratedProperties(
updatePayload = [CHILDREN, '' + nextProp];
}
}
} else if (registrationNameModules.hasOwnProperty(propKey)) {
} else if (propKey[0] === 'o' && propKey[1] === 'n') {
if (nextProp != null) {
if (__DEV__ && typeof nextProp !== 'function') {
warnForInvalidEventListener(propKey, nextProp);
Expand Down
11 changes: 10 additions & 1 deletion packages/react-dom/src/events/ReactBrowserEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ let reactTopListenersCounter = 0;
*/
const topListenersIDKey = '_reactListenersID' + ('' + Math.random()).slice(2);

function getEventsFromRegistraionName(registrationName) {
if (!(registrationName in registrationNameDependencies)) {
registrationNameDependencies[registrationName] = [
`top${registrationName.slice(2)}`,
Copy link
Contributor

@nhunzaker nhunzaker Jan 8, 2018

Choose a reason for hiding this comment

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

Purely for discussion but...

It's really a bummer we need top level event name constants period. There's nothing special about them. We could probably just use the event name that comes from the DOM.

If we dropped top level event name constants, we could avoid binding all events, eliminating overhead for local listeners:
https://github.com/facebook/react/blob/master/packages/react-dom/src/events/ReactDOMEventListener.js#L126

Maybe that's too aggressive. 🤷‍♂️

Copy link
Contributor Author

@jquense jquense Jan 8, 2018

Choose a reason for hiding this comment

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

Yeah I was thinking the same thing. There doesn't seem to be much value to them they are just the event name with 'top' appended.

Copy link
Contributor

@aweary aweary Jan 12, 2018

Choose a reason for hiding this comment

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

It depends on how it affects bundle size. If we keep them and do something like #11894 then the event plugins can just use the numeric constants for dependencies/switching on events, which would probably compress better.

];
}
return registrationNameDependencies[registrationName];
}

function getListeningForDocument(mountAt) {
// In IE8, `mountAt` is a host object and doesn't have `hasOwnProperty`
// directly.
Expand Down Expand Up @@ -115,7 +124,7 @@ function getListeningForDocument(mountAt) {
export function listenTo(registrationName, contentDocumentHandle) {
const mountAt = contentDocumentHandle;
const isListening = getListeningForDocument(mountAt);
const dependencies = registrationNameDependencies[registrationName];
const dependencies = getEventsFromRegistraionName(registrationName);
Copy link
Contributor

@nhunzaker nhunzaker Jan 8, 2018

Choose a reason for hiding this comment

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

Registration!


for (let i = 0; i < dependencies.length; i++) {
const dependency = dependencies[i];
Expand Down
Loading