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

Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) #23386

Merged

Conversation

JoshuaGross
Copy link
Contributor

@JoshuaGross JoshuaGross commented Feb 28, 2022

Summary

This is a resubmission of #23278 but only supporting the add/removeEventListener APIs for Fabric only.

  1. We currently provide no /stable/ interface, this may change
  2. We currently provide no interface for non-Fabric
  3. All events emitted into the system are prefixed with rn: to indicate that none of these events are, necessarily, W3C-compliant (for example, scroll or touch events might deviate from the spec) -- as we support specs explicitly, those events would be emitted non-prefixed
  4. Event and CustomEvent implementations to be provided by React Native, and are assumed to be on the global scope

Followup

  1. Make HostComponent JS wrapper lazily initialized

How did you test this change?

see internal tests

@sizebot
Copy link

sizebot commented Feb 28, 2022

Comparing: a232744...15fc32a

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.31 kB 131.31 kB = 42.00 kB 42.00 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 135.96 kB 135.96 kB = 43.38 kB 43.38 kB
facebook-www/ReactDOM-prod.classic.js = 434.31 kB 434.31 kB = 79.46 kB 79.46 kB
facebook-www/ReactDOM-prod.modern.js = 420.82 kB 420.82 kB = 77.43 kB 77.44 kB
facebook-www/ReactDOMForked-prod.classic.js = 434.31 kB 434.31 kB = 79.46 kB 79.46 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/implementations/ReactFabric-prod.js +1.72% 274.61 kB 279.33 kB +2.23% 49.51 kB 50.61 kB
react-native/implementations/ReactFabric-prod.fb.js +1.65% 286.09 kB 290.81 kB +2.19% 51.67 kB 52.80 kB
react-native/implementations/ReactFabric-profiling.js +1.61% 293.61 kB 298.32 kB +2.15% 52.68 kB 53.82 kB
react-native/implementations/ReactFabric-profiling.fb.js +1.51% 313.04 kB 317.76 kB +2.08% 55.92 kB 57.08 kB
react-native/implementations/ReactFabric-dev.js +1.28% 728.03 kB 737.32 kB +2.08% 158.76 kB 162.06 kB
react-native/implementations/ReactFabric-dev.fb.js +1.21% 770.24 kB 779.53 kB +1.99% 166.51 kB 169.82 kB
react-native/implementations/ReactNativeRenderer-prod.js +1.09% 283.23 kB 286.32 kB +1.45% 51.07 kB 51.81 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js +1.06% 290.05 kB 293.13 kB +1.37% 52.55 kB 53.28 kB
react-native/implementations/ReactNativeRenderer-profiling.js +1.02% 302.27 kB 305.35 kB +1.33% 54.23 kB 54.95 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js +0.97% 316.89 kB 319.97 kB +1.29% 56.70 kB 57.43 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.77% 740.42 kB 746.09 kB +1.47% 161.49 kB 163.87 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js +0.73% 780.77 kB 786.44 kB +1.41% 168.81 kB 171.19 kB

Generated by 🚫 dangerJS against 15fc32a

}

// TODO: optimize this path to make remove cheaper
eventListeners[eventType] = namedEventListeners.filter(listenerObj => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be optimized, but (since the API is unstable, not used at all, and this change is primarily for prototyping) I would prefer not spending too much time on this atm until we we've validated the usefulness of these APIs

);

// Avoid allocating additional arrays here
if (event._dispatchInstances == null && listenersLength === 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this stuff seems a bit convoluted: the goal is to avoid array allocation at all costs since this is a very hot path. See accumulateInto - we do it manually out here for _dispatchInstances to avoid calling accumulateInto N times or allocating an array of insts, which I did previously.

@JoshuaGross
Copy link
Contributor Author

This has been tested, optimized, is working well in my tests, and should be ready for review now.

// Thus, we prefix to ensure no collision with W3C event names.
const requestedPhaseIsCapture = phase === 'captured';
const mangledImperativeRegistrationName = requestedPhaseIsCapture
? 'rn:' + registrationName.replace(/Capture$/, '')
Copy link
Contributor

@yungsters yungsters Mar 1, 2022

Choose a reason for hiding this comment

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

Alternatively, this could be invariant(registrationName.endsWith('Capture'), …), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so (as discussed over VC) - do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The invariant would succeed probably 99% of the time though

const eventListeners =
stateNode.canonical._eventListeners[mangledImperativeRegistrationName];

eventListeners.forEach(listenerObj => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider filtering this down and then deciding what to do with the return value (e.g. how to incorporate listener).

const eventListeners =
stateNode.canonical._eventListeners[mangledImperativeRegistrationName];

eventListeners.forEach(listenerObj => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are worried about extra allocations, maybe replace this with a for-loop.

}

// Remove from the event listener once it's been called
stateNode.canonical.removeEventListener_unstable(
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to invoke this beforehand, in case the listener throws.

// all imperative event listeners in a function to unwrap the SyntheticEvent
// and pass them an Event.
// When this API is more stable and used more frequently, we can revisit.
const listenerFnWrapper = function(syntheticEvent) {
Copy link
Contributor

@lunaleaps lunaleaps Mar 2, 2022

Choose a reason for hiding this comment

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

How come we don't grab the args on this function to pass on line 130, like we do in the case of line 138?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory this is the only argument and we need to do something with specificEvent... in the code below, I don't really need to do anything with the args so I just pass them on without assuming anything. Practically it could be event instead of ...args below, but theoretically ...args is more flexible? Doesn't really matter, just personal preference I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I wonder if they should be consistent? Like

function(syntheticEvent, ...args) {
...
... listener(customEvent, ...args);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunaleaps how's the current impl?

I don't want to pull out event where it's not actually used, but it makes sense to always have rest/spread args.

@lunaleaps
Copy link
Contributor

Yup still lgtm! :shipit:

@JoshuaGross JoshuaGross merged commit 05c283c into facebook:main Mar 2, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 24, 2022
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
… (unstable only) (facebook#23386)

* Implement addEventListener and removeEventListener on Fabric HostComponent

* add files

* re-add CustomEvent

* fix flow

* Need to get CustomEvent from an import since it won't exist on the global scope by default

* yarn prettier-all

* use a mangled name consistently to refer to imperatively registered event handlers

* yarn prettier-all

* fuzzy null check

* fix capture phase event listener logic

* early exit from getEventListeners more often

* make some optimizations to getEventListeners and the bridge plugin

* fix accumulateInto logic

* fix accumulateInto

* Simplifying getListeners at the expense of perf for the non-hot path

* feedback

* fix impl of getListeners to correctly remove function

* pass all args in to event listeners
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
sammy-SC pushed a commit that referenced this pull request Mar 2, 2023
…derer (#26282)

## Summary

I'm going to start implementing parts of this proposal
react-native-community/discussions-and-proposals#607

As part of that implementation I'm going to refactor a few parts of the
interface between React and React Native. One of the main problems we
have right now is that we have private parts used by React and React
Native in the public instance exported by refs. I want to properly
separate that.

I saw that a few methods to attach event handlers imperatively on refs
were also exposing some things in the public instance (the
`_eventListeners`). I checked and these methods are unused, so we can
just clean them up instead of having to refactor them too. Adding
support for imperative event listeners is in the roadmap after this
proposal, and its implementation might differ after this refactor.

This is essentially a manual revert of #23386.

I'll submit more PRs after this for the rest of the refactor.

## How did you test this change?

Existing jest tests. Will test a React sync internally at Meta.
github-actions bot pushed a commit that referenced this pull request Mar 2, 2023
…derer (#26282)

## Summary

I'm going to start implementing parts of this proposal
react-native-community/discussions-and-proposals#607

As part of that implementation I'm going to refactor a few parts of the
interface between React and React Native. One of the main problems we
have right now is that we have private parts used by React and React
Native in the public instance exported by refs. I want to properly
separate that.

I saw that a few methods to attach event handlers imperatively on refs
were also exposing some things in the public instance (the
`_eventListeners`). I checked and these methods are unused, so we can
just clean them up instead of having to refactor them too. Adding
support for imperative event listeners is in the roadmap after this
proposal, and its implementation might differ after this refactor.

This is essentially a manual revert of #23386.

I'll submit more PRs after this for the rest of the refactor.

## How did you test this change?

Existing jest tests. Will test a React sync internally at Meta.

DiffTrain build for [d49e0e0](d49e0e0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants