Skip to content

Use proper enums to communicate between JS and C++ in NativePerformanceObserver#36156

Closed
rshest wants to merge 5 commits into
facebook:mainfrom
rshest:export-D43292282
Closed

Use proper enums to communicate between JS and C++ in NativePerformanceObserver#36156
rshest wants to merge 5 commits into
facebook:mainfrom
rshest:export-D43292282

Conversation

@rshest
Copy link
Copy Markdown
Contributor

@rshest rshest commented Feb 14, 2023

Summary:
[Changelog][Internal]

After D42884147 (ceb1d0d) (#36030) had landed, we now can use enums as data types when bridging between JS and C++ TurboModules.

This diff takes advantage of this and converts the API to use the proper enums instead of int types that it was forced to use earlier.

NOTE: This also relies on additional change to codegen, D43292254, which is required in order for enums to work also as part of data structures.

Differential Revision: D43292282

rshest and others added 5 commits February 14, 2023 15:54
…ok#36116)

Summary:
Pull Request resolved: facebook#36116

[Changelog][Internal]

Add a minimal/reference JavaScript implementation for NativePerformanceObserver - the purpose is both unit testing (JS and native sides separately) and potentially shimming the part of functionality that is not dependent on native side.

This is both a setup for adding general unit tests for the Performance* APIs, but also to be able to do non-trivial changes on JS side for WebPerformance (such as in (D43154319).

Differential Revision: https://internalfb.com/D43167392

fbshipit-source-id: 19b75cbbf27be59bf86dbf1cda3acb863bf89b59
Summary:
[Changelog][Internal]

By [the W3C standard](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceObserver/observe), `PerformanceObserver.observer` can optionally take a `durationThreshold` option, so that only entries with duration larger than the threshold are reported.

This diff adds support for this on the RN side.

NOTE: The standard suggests that default value for this is 104s. I left it at 0 for now, as for the RN use cases t may be to too high (needs discussion).

Differential Revision: https://internalfb.com/D43154319

fbshipit-source-id: a3ace67db01dbd9f414eff2f3028284cea3e615c
Summary:
[Changelog][Internal]

Implements EventCounts API (`Performance.eventCounts`) for Web Performance, according to the W3C standard: https://www.w3.org/TR/event-timing/#eventcounts

Differential Revision: https://internalfb.com/D43285073

fbshipit-source-id: 5e923b7a4915968ecf210ba8ed3951c637d9c5e5
…degen

Differential Revision: D43292254

fbshipit-source-id: 456902763d0b8232c6fca7a36dd442d200caa8d5
…ceObserver

Summary:
[Changelog][Internal]

After D42884147 (facebook@ceb1d0d) (facebook#36030) had landed, we now can use enums as data types when bridging between JS and C++ TurboModules.

This diff takes advantage of this and converts the API to use the proper enums instead of int types that it was forced to use earlier.

NOTE: This also relies on additional change to codegen, D43292254, which is required in order for enums to work also as part of data structures.

Differential Revision: D43292282

fbshipit-source-id: fd271704ab60aec2b3ae5b260e76665ad50e206e
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels Feb 14, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D43292282


import NativePerformanceObserver from './NativePerformanceObserver';
import {warnNoNativePerformanceObserver} from './PerformanceObserver';
import {rawToPerformanceEntryType} from './RawPerformanceEntry';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • ⚠️ Libraries/WebPerformance/EventCounts.js line 13 – 'rawToPerformanceEntryType' is defined but never used. (no-unused-vars)

@github-actions
Copy link
Copy Markdown

Fails
🚫

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

DetailsCATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 2b95380

@analysis-bot
Copy link
Copy Markdown

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,485,991 +418
android hermes armeabi-v7a 7,806,753 +391
android hermes x86 8,963,273 +466
android hermes x86_64 8,820,226 +435
android jsc arm64-v8a 6,699,718 +429
android jsc armeabi-v7a 7,491,192 +399
android jsc x86 9,224,386 +461
android jsc x86_64 6,924,989 +461

Base commit: 11570e7
Branch: main

@github-actions
Copy link
Copy Markdown

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Aug 14, 2023
@github-actions
Copy link
Copy Markdown

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions Bot closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported p: Facebook Partner: Facebook Partner Stale There has been a lack of activity on this issue and it may be closed soon.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants