-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add React Native perf metrics behind flag #16
Add React Native perf metrics behind flag #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please see the comment about code paths for Meta's license headers
front_end/core/host/RNPerfMetrics.ts
Outdated
// Copyright 2024 The Chromium Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
// Copyright (c) Meta Platforms, Inc. and affiliates. | ||
// Copyright 2020 The Chromium Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add this new file (and other) to
rn-chrome-devtools-frontend/scripts/eslint_rules/lib/check_license_header.js
Lines 69 to 72 in 8c5e07a
const META_CODE_PATHS = [ | |
'entrypoints/rn_inspector', | |
'panels/rn_welcome', | |
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Some eslint rule was looking for addEventListener
and expects a second param. That crashed so the license linter didn't run either
@@ -69,6 +69,7 @@ const EXCLUDED_FILES = [ | |||
const META_CODE_PATHS = [ | |||
'entrypoints/rn_inspector', | |||
'panels/rn_welcome', | |||
'core/host/RNPerfMetrics.ts', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EdmondChuiHW consider adding front_end/global_typings/react_native.d.ts
here as well in one of the next PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Looks like .d.ts
are excluded somehow. The upstream file here doesn't have license and the linter doesn't complain.
type RequestIdleCallbackHandle = any; |
Added ours to the list for clarity in #18
Summary
This PR enables performance metrics to be collected in internal builds of CDT Frontend for performance, reliability, and efficiency.
embedderScript.js
embedderScript.js
is now adefer
script.type="module"
script isdefer
by definition, the order of these script tags in HTML will be the order of execution per spec.With the flag enabled, you can use
addListener
to handle events:Planned in next PRs
sendEvent(…)
(no-op when flag is false/missing)Test plan
In both release and default builds:
Expect console to print
TestyMcTest
Upstreaming plan
devtools-frontend
repo. I've reviewed the contribution guide.References
UserMetrics
:rn-chrome-devtools-frontend/front_end/core/host/UserMetrics.ts
Line 34 in 9370b35