Refactor createEventHandle signature #19174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
Details of bundled changes.Comparing: 97b96da...eff2510 react-dom
react-art
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 97b96da...eff2510 react-dom
react-art
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
trueadm
force-pushed
the
revise-create-event-handle
branch
3 times, most recently
from
fa15c98 to
4125918
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit eff2510:
|
bvaughn
reviewed
Jun 22, 2020
bvaughn
approved these changes
Jun 22, 2020
packages/react-interactions/events/src/dom/create-event-handle/useEvent.js
Show resolved
Hide resolved
packages/react-interactions/events/src/dom/create-event-handle/useEvent.js
Show resolved
Hide resolved
trueadm
force-pushed
the
revise-create-event-handle
branch
from
68af22c to
be0c79a
Compare
Delete target Delete target Add back codes Revise
trueadm
force-pushed
the
revise-create-event-handle
branch
from
be0c79a to
eff2510
Compare
gaearon
reviewed
Aug 24, 2020
| if (targetListeners !== null) { | ||
| targetListeners.delete(listener); | ||
| } | ||
| const registeredReactDOMEvents = new PossiblyWeakSet(); |
There was a problem hiding this comment.
Does this mean we leak memory on IE11? It looks like we always add targets to this Set, but it's never cleared (since it's assumed to be a WeakSet in modern browsers).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the
ReactDOM.createEventHandleAPI so that the signature is simpler and more barebones. Previously,createEventHandlereturned an object with the methodssetListenerandclear, which mimiced whatuseEventdoes.After utilizing this API internally, primarily behind
useEvent, it makes little sense forcreateEventHandleto have this signature. It still makses sense foruseEventto have this signtuare, but it's not important that the core API works the same. All we really want this core API to do is add an event and remove and event – sort of like a better nativeaddEventListener.This change means we no longer return an object from
createEventHandle, but rather a function that sets the listener. Upon using the setter, you get back an unsubscribe/clear listener function.To show a more concrete example, previously you might use the API in this way:
With the changes in this PR, the above would look like this instead:
This change offers some benefits:
createEventHandleby not having as much overhead