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

React Native raw event EventEmitter - intended for app-specific perf listeners and debugging #23232

Merged

Conversation

JoshuaGross
Copy link
Contributor

Summary

This is a replacement / alternative to the Pressability-based event telemetry system that has existed since ~1 year ago. Instead of relying on the internals of the Pressability state machine, and only being able to instrument certain touch events, this system will allow us to instrument any/all events that flow through React Native into ReactJS.

How did you test this change?

Testing manually on an internal stack, with a very small interface.

…utside of Pressability to capture all touch events, and other event types
@sizebot
Copy link

sizebot commented Feb 4, 2022

Comparing: 5318971...143b04b

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 = 129.75 kB 129.75 kB = 41.59 kB 41.59 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.93 kB 134.93 kB = 43.12 kB 43.12 kB
facebook-www/ReactDOM-prod.classic.js = 428.75 kB 428.75 kB = 78.70 kB 78.70 kB
facebook-www/ReactDOM-prod.modern.js = 418.74 kB 418.74 kB = 77.25 kB 77.25 kB
facebook-www/ReactDOMForked-prod.classic.js = 428.75 kB 428.75 kB = 78.70 kB 78.70 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-dev.js +0.22% 719.74 kB 721.32 kB +0.38% 156.53 kB 157.13 kB
react-native/implementations/ReactFabric-dev.fb.js +0.21% 762.00 kB 763.58 kB +0.36% 164.29 kB 164.88 kB

Generated by 🚫 dangerJS against 143b04b

@gaearon
Copy link
Collaborator

gaearon commented Feb 4, 2022

Is there a way to catch these before they flow into React? We’ve been trying to gradually move away from this idea of a “plugin system” so adding more of these seems not great.

@JoshuaGross
Copy link
Contributor Author

cc @yungsters any ideas? I think React sees these events before anything sees them in RN?

@JoshuaGross
Copy link
Contributor Author

@gaearon Also, anywhere I can read more about this effort? The plugin system in RN seems to have been untouched, largely, for the past 5+ years, and I wasn't aware of any efforts to change it one way or another.

@yungsters
Copy link
Contributor

The first bit of JavaScript that executes in response to an event is dispatchEvent in ReactFabricEventEmitter:

export function dispatchEvent(
target: null | Object,
topLevelType: TopLevelType,
nativeEvent: AnyNativeEvent,
) {
const targetFiber = (target: null | Fiber);
let eventTarget = null;
if (targetFiber != null) {
const stateNode = targetFiber.stateNode;
// Guard against Fiber being unmounted
if (stateNode != null) {
eventTarget = stateNode.canonical;
}
}
batchedUpdates(function() {
// Heritage plugin event system
runExtractedPluginEventsInBatch(
topLevelType,
targetFiber,
nativeEvent,
eventTarget,
);
});
// React Native doesn't use ReactControlledComponent but if it did, here's
// where it would do it.
}

@gaearon — Are you asking whether there is something that executes before the Event Plugin System? If so, would emitting these events before runExtractedPluginEventsInBatch in that function (that I linked to) address your concerns?

The main upside of doing this ^ would be to avoid conforming more logic to the current Event Plugin System. But for the relatively low complexity of what @JoshuaGross introduces in this pull request, I don't think it is a big deal either way. (This extra plugin is not going to make it much harder for anyone to propose and build a replacement.)

import {RawEventTelemetryEventEmitter} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface';

const ReactNativeEventTelemetryPlugin = {
extractEvents: function(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Concise notation.

Suggested change
extractEvents: function(
extractEvents(

@gaearon
Copy link
Collaborator

gaearon commented Feb 4, 2022

If so, would emitting these events before runExtractedPluginEventsInBatch in that function (that I linked to) address your concerns?

Yes, I would prefer that.

…ntEmitter, to reduce reliance on the plugin system and move the emit call further into the core
@JoshuaGross
Copy link
Contributor Author

Thanks for the feedback @gaearon @yungsters, I backed-out changes to the Plugin system and I'm emitting the event from ReactFabricEventEmitter now.

@JoshuaGross JoshuaGross changed the title WIP: React Native raw event telemetry plugin React Native raw event telemetry plugin Feb 7, 2022
// to be notified for all events.
// Note that extracted events are *not* emitted into the telemetry system,
// only events that have a 1:1 mapping with a native event, at least for now.
const topLevelTypeStr: string = ((topLevelType: any): string);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we name this somehow to make it clearer this is opt-in and that RN does not collect any telemetry by default? I can absolutely see someone “finding” it in code and writing an HN article about it. Same goes for the comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: RawEventTelemetryOffByDefault :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an extremely valid concern.

@JoshuaGross
Copy link
Contributor Author

Are the comments I added enough? I'm sort of happy to rename everything to RawEventTelemetryEventEmitterOffByDefault but this name sucks a lot, and will be confusing when it is in use. cc @gaearon @yungsters

@JoshuaGross JoshuaGross changed the title React Native raw event telemetry plugin React Native raw event EventEmitter - intended for app-specific perf listeners and debugging Feb 8, 2022
@JoshuaGross JoshuaGross merged commit 9d4e8e8 into facebook:main Feb 8, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 9, 2022
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([#23236](facebook/react#23236))" ([#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
…listeners and debugging (facebook#23232)

* RawEventEmitter: new event perf profiling mechanism outside of Pressability to capture all touch events, and other event types

* sync

* concise notation

* Move event telemetry event emitter call from Plugin to ReactFabricEventEmitter, to reduce reliance on the plugin system and move the emit call further into the core

* Backout changes to ReactNativeEventPluginOrder

* Properly flow typing event emitter, and emit event to two channels: named and catchall

* fix typing for event name string

* fix typing for event name string

* fix flow

* Add more comments about how the event telemetry system works

* Add more comments about how the event telemetry system works

* rename to RawEventTelemetryEventEmitterOffByDefault

* yarn prettier-all

* rename event

* comments

* improve flow types

* renamed file
nevilm-lt pushed a commit to nevilm-lt/react that referenced this pull request Apr 22, 2022
…listeners and debugging (facebook#23232)

* RawEventEmitter: new event perf profiling mechanism outside of Pressability to capture all touch events, and other event types

* sync

* concise notation

* Move event telemetry event emitter call from Plugin to ReactFabricEventEmitter, to reduce reliance on the plugin system and move the emit call further into the core

* Backout changes to ReactNativeEventPluginOrder

* Properly flow typing event emitter, and emit event to two channels: named and catchall

* fix typing for event name string

* fix typing for event name string

* fix flow

* Add more comments about how the event telemetry system works

* Add more comments about how the event telemetry system works

* rename to RawEventTelemetryEventEmitterOffByDefault

* yarn prettier-all

* rename event

* comments

* improve flow types

* renamed file
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Summary:
This sync includes the following changes:
- **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([facebook#23258](facebook/react#23258)) //<Sebastian Markbåge>//
- **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([facebook#23247](facebook/react#23247)) //<Sebastian Markbåge>//
- **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([facebook#23257](facebook/react#23257)) //<Sebastian Markbåge>//
- **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([facebook#23232](facebook/react#23232)) //<Joshua Gross>//
- **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([facebook#23241](facebook/react#23241)) //<salazarm>//
- **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([facebook#23236](facebook/react#23236))" ([facebook#23239](facebook/react#23239)) //<dan>//
- **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([facebook#23236](facebook/react#23236)) //<sunderls>//
- **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([facebook#23207](facebook/react#23207)) //<Andrew Clark>//
- **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([facebook#23227](facebook/react#23227)) //<Andrew Clark>//
- **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz  ([facebook#23224](facebook/react#23224)) //<salazarm>//
- **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>//
- **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>//
- **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([facebook#23176](facebook/react#23176)) //<salazarm>//
- **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([facebook#23171](facebook/react#23171)) //<Fran Dios>//
- **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([facebook#23151](facebook/react#23151)) //<Brian Vaughn>//
- **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([facebook#23150](facebook/react#23150)) //<Douglas Armstrong>//
- **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([facebook#23145](facebook/react#23145)) //<Dan Abramov>//
- **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([facebook#23095](facebook/react#23095)) //<Dan Abramov>//
- **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([facebook#23142](facebook/react#23142)) //<Brian Vaughn>//
- **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([facebook#23111](facebook/react#23111)) //<Dan Abramov>//

Changelog:
[General][Changed] - React Native sync for revisions 51947a1...a3bde79

jest_e2e[run_all_tests]

Reviewed By: ShikaSD

Differential Revision: D34108924

fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
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