[WIP] Remove event whitelist #11992
[WIP] Remove event whitelist #11992
Conversation
|
we could also remove some complexity by not having these separate topLevelTypes and use the event name (maybe lowercased?) |
| if (!topLevelEventsToDispatchConfig[topLevelType]) { | ||
| const onEvent = `on${topLevelType.slice(3)}`; | ||
| topLevelEventsToDispatchConfig[topLevelType] = { | ||
| phasedRegistrationNames: { |
There was a problem hiding this comment.
this could be simplified to just dispatchType phased/direct if the names don't need to be configurable. I think on the DOM side the convention hold everywhere, not sure about RN tho
|
Sorry, forgot to actually add my thoughts about this: The idea is that |
| function getEventsFromRegistraionName(registrationName) { | ||
| if (!(registrationName in registrationNameDependencies)) { | ||
| registrationNameDependencies[registrationName] = [ | ||
| `top${registrationName.slice(2)}`, |
There was a problem hiding this comment.
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. 🤷♂️
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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); | |||
|
What is the file size impact? |
|
at the moment it's the same essentially, when gzipped, but does have the benefit of not having to add more config for each new DOM event. Having looked through a bit more, the stumbling block for ripped more config out is the mapping between DOM event and prop name. One way to handle this is could be to build a mapping as we go in DOMFiberComponent, then map back in eventPropagators. That should mean we can remove all the explicit registration names, but still be able to handle renderer specific mappings like prefixed animation events. I'm really not sure tho if RN is as consistent as RD on this tho, RD has little need for the level of config the event system offers. |
| @@ -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') { | |||
There was a problem hiding this comment.
It might be worth extracting this into a function like isEventName so it minifies better.
|
@jquense Any updates here, or desire to pick this up again? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Not working quite yet, but making some progress. So far the dodgiest thing here is the mapping from Synthetic event to native event. I’m not sure that
event.constructor.nameis gonna cut it across all browsers.There is potentially a lot of room for simplify the event dispatch config, if we insist on the convention of
onXandonXCapture, you can remove most if not all of the registration name stuff, and map between the types as you go. We do lose the dev warnings about event names but that could be added back as a dev only injection