Pass mutation list to RCTMountingTransactionObserving callbacks #33510

kmagiera · 2022-03-28T14:07:58Z

Summary

This PR updates RCTMountingTransactionObserving protocol to accept full MountingTransaction object as an argument to its methods as opposed to just MountingTransactionMetdata which contained only some subset of information.

This change makes it possible for components implementing the protocol to analyze the list of mutations and hence only take action when certain mutations are about to happen.

One of the use cases for RCTMountingTransactionObserving protocol is to allow for Modal to take view snapshot before it is closed, such that an animated close transition can be performed over the snapshotted content. Note that when modal is removed from the view hierarchy its children are gone too and therefore the snapshot mechanism makes it possible for children to still be visible while the animated closing transition is ongoing. A similar use-case to that can be seen in react-native-screens library, where we use the same snapshot mechanism for views that are removed from navigation stack.

Before this change, we'd use mountingTransactionDidMountWithMetadata to take a snapshot. However, making a snapshot of relatively complex view hierarchy can be expensive, and we'd like to make sure that we only perform a snapshot when the given modal is about to be removed. Because the mentioned method does not provide an information about what changes are going to be performed in a given transaction, we'd make the snapshot for every single view transaction that happens while the modal is mounted.

In this PR we're updating RCTMountingTransactionObserving protocol's methods, in particular, we rename methods to no longer contain "Metadata" in them and to accept MountingTransaction as the only argument instead of MountingTransactionMetadata object. With this change we are also deleting MountingTransactionMetadata altogether as it has no uses outside the protocol. Finally, we update the two uses of the protocol in RCTScrollViewComponentView and RCTModalHostViewComponentView.

Changelog

[iOS][Fabric] - Update RCTMountingTransactionObserving protocol to consume MountingTransaction objects

Test Plan

As there are not that many uses of RCTMountingTransactionObserving protocol during testing I focused on checking if the updated method is called and if the provided objects contains the proper data. Unfortunately, despite code for the modal protocol being present in OSS version it does seem like some parts of modal implementation are still missing and the component only renders an unimplemented view (checked this with rn-tester). I only managed to verify the use in RCTScrollViewComponentView with the following steps:

Build for iOS
Put a breakpoint in mountingTransactionDidMount method in RCTScrollViewComponentView.mm
Verify that the program stops on the breakpoint when a scrollview is rendered (use any screen on rn-tester app)
Inspect the provided object in the debugger (ensure the list of transactions is not empty)

Outside of that we verified the transactions can be processed in mountingTransactionDidMount after the changes from this PR are applied in FabricExample app in react-native-screens repo.

…llback

pull-bot · 2022-03-28T14:12:45Z

	Messages
📖	📋 Verify Changelog Format - A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY 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 f03b034

analysis-bot · 2022-03-28T14:23:32Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 6e5cefe
Branch: main

analysis-bot · 2022-03-29T08:14:52Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,765,986	-499
android	hermes	armeabi-v7a	7,175,065	-341
android	hermes	x86	8,073,549	-607
android	hermes	x86_64	8,052,686	-643
android	jsc	arm64-v8a	9,643,229	-496
android	jsc	armeabi-v7a	8,417,616	-348
android	jsc	x86	9,592,307	-599
android	jsc	x86_64	10,189,448	-651

Base commit: 6e5cefe
Branch: main

ShikaSD

looks good! a few questions to understand the changes better :)

ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm

facebook-github-bot · 2022-03-29T12:30:34Z

@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-04-01T10:31:08Z

@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…asses

ShikaSD · 2022-04-01T15:23:51Z

packages/rn-tester/Podfile.lock

@@ -890,8 +890,8 @@ SPEC CHECKSUMS:
  boost: a7c83b31436843459a1961bfd74b96033dc77234
  CocoaAsyncSocket: 065fd1e645c7abab64f7a6a2007a48038fdc6a99
  DoubleConversion: 5189b271737e1565bdce30deb4a08d647e3f5f54
-  FBLazyVector: 19e408e76fa9258dd32191a50d60c41444f52d29


I think you can revert that, CI passes now with the internal cache update :)

ShikaSD · 2022-04-01T15:26:33Z

ReactCommon/react/renderer/mounting/TelemetryController.cpp

  telemetry.didMount();

  compoundTelemetry.incorporate(telemetry, numberOfMutations);

-  didMount({surfaceId, number, telemetry, compoundTelemetry});


I started updating internal usages of this method, seems like compoundTelemetry is not available in the transaction .
I am not sure what's the best course of action here, maybe we could pass both transaction and telemetry as separate parameters or rollback deletion of metadata and just have it as a { transaction, compoundTelemetry } type of object?

How about passing SurfaceTelemetry as a second argument to all the observer callbacks? For me it'd seem more natural than adding additional struct to carry them both. If you'd prefer to have such struct then I'd consider either naming it differently than "*Metadata" as it'd seem unintuitive that "TransactionMetadata" actually carries "Transaction". Alternatively I can revert "Metadata" to its previous form and add only a reference to "mutations" to it as that's the only thing we need from transaction to fulfil out usecase.

Yep, SurfaceTelemetry as a separate parameter sounds good to me :)

…er callbacks

…llback

kmagiera · 2022-04-05T15:20:31Z

Hi @ShikaSD – I updated observer to pass presenter ref as discussed and also reverted changes to Podfile. The CI failure seem unrelated to this PR.

facebook-github-bot · 2022-04-20T12:14:28Z

@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

react-native-bot · 2022-04-21T12:15:04Z

This pull request was successfully merged by @kmagiera in 91fc2c0.

^{When will my fix make it into a release? | Upcoming Releases}

facebook-github-bot · 2022-04-21T23:32:06Z

This pull request has been reverted by 2f5a1e6.

…acks" Summary: https://fb.workplace.com/groups/fbapp.commerce.engsupport/permalink/2074812256012212/ Back out "[react-native][PR] Pass mutation list to RCTMountingTransactionObserving callbacks" Original commit changeset: f40afc512f2c Original Phabricator Diff: D35214478 (91fc2c0) Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D35825832 fbshipit-source-id: b53b616dca39c84b3a8e8e4cbaa4a45834e53fe3

…book#33510) Summary: This PR updates `RCTMountingTransactionObserving` protocol to accept full `MountingTransaction` object as an argument to its methods as opposed to just `MountingTransactionMetdata` which contained only some subset of information. This change makes it possible for components implementing the protocol to analyze the list of mutations and hence only take action when certain mutations are about to happen. One of the use cases for `RCTMountingTransactionObserving` protocol is to allow for Modal to take view snapshot before it is closed, such that an animated close transition can be performed over the snapshotted content. Note that when modal is removed from the view hierarchy its children are gone too and therefore the snapshot mechanism makes it possible for children to still be visible while the animated closing transition is ongoing. A similar use-case to that can be seen in react-native-screens library, where we use the same snapshot mechanism for views that are removed from navigation stack. Before this change, we'd use `mountingTransactionDidMountWithMetadata` to take a snapshot. However, making a snapshot of relatively complex view hierarchy can be expensive, and we'd like to make sure that we only perform a snapshot when the given modal is about to be removed. Because the mentioned method does not provide an information about what changes are going to be performed in a given transaction, we'd make the snapshot for every single view transaction that happens while the modal is mounted. In this PR we're updating `RCTMountingTransactionObserving` protocol's methods, in particular, we rename methods to no longer contain "Metadata" in them and to accept `MountingTransaction` as the only argument instead of `MountingTransactionMetadata` object. With this change we are also deleting `MountingTransactionMetadata` altogether as it has no uses outside the protocol. Finally, we update the two uses of the protocol in `RCTScrollViewComponentView` and `RCTModalHostViewComponentView`. [iOS][Fabric] - Update RCTMountingTransactionObserving protocol to consume MountingTransaction objects Pull Request resolved: facebook#33510 Test Plan: As there are not that many uses of `RCTMountingTransactionObserving` protocol during testing I focused on checking if the updated method is called and if the provided objects contains the proper data. Unfortunately, despite code for the modal protocol being present in OSS version it does seem like some parts of modal implementation are still missing and the component only renders an unimplemented view (checked this with rn-tester). I only managed to verify the use in `RCTScrollViewComponentView` with the following steps: 1. Build for iOS 2. Put a breakpoint in mountingTransactionDidMount method in `RCTScrollViewComponentView.mm` 3. Verify that the program stops on the breakpoint when a scrollview is rendered (use any screen on rn-tester app) 4. Inspect the provided object in the debugger (ensure the list of transactions is not empty) Outside of that we verified the transactions can be processed in `mountingTransactionDidMount` after the changes from this PR are applied in FabricExample app in [react-native-screens](https://github.com/software-mansion/react-native-screens/tree/main/FabricExample) repo. Reviewed By: cipolleschi Differential Revision: D35214478 Pulled By: ShikaSD fbshipit-source-id: f40afc512f2c8cfa6262d2fb82fb1ccb05aa734c

kmagiera added 3 commits February 22, 2022 15:23

Stashing work

f840a22

Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…

33d26c3

…llback

Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…

d125f38

…llback

Drop unused variables

edff89a

ShikaSD reviewed Mar 29, 2022

View reviewed changes

ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h Outdated Show resolved Hide resolved

React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm Outdated Show resolved Hide resolved

kmagiera added 2 commits April 1, 2022 12:24

Remove MountingTransactionMetadata

f2a6105

Revert changes in mounting observer for modal component

124c731

Commit podfile lock changes (perhaps not a valid ones) to see if CI p…

87084d0

…asses

ShikaSD reviewed Apr 1, 2022

View reviewed changes

kmagiera added 3 commits April 4, 2022 10:29

Revert changes in rn-tester's podfile.lock

d08c67f

Add SurfaceTelemetry as additional argument to all transaction observ…

d9b3edb

…er callbacks

Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…

f03b034

…llback

facebook-github-bot closed this in 91fc2c0 Apr 21, 2022

react-native-bot added the Merged This PR has been merged. label Apr 21, 2022

facebook-github-bot added the Reverted label Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

kmagiera commented Mar 28, 2022

pull-bot commented Mar 28, 2022 •

edited

Loading

analysis-bot commented Mar 28, 2022 •

edited

Loading

analysis-bot commented Mar 29, 2022 •

edited

Loading

ShikaSD left a comment

facebook-github-bot commented Mar 29, 2022

facebook-github-bot commented Apr 1, 2022

ShikaSD Apr 1, 2022

ShikaSD Apr 1, 2022

kmagiera Apr 4, 2022

ShikaSD Apr 4, 2022

kmagiera commented Apr 5, 2022

facebook-github-bot commented Apr 20, 2022

react-native-bot commented Apr 21, 2022

facebook-github-bot commented Apr 21, 2022

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Conversation

kmagiera commented Mar 28, 2022

Summary

Changelog

Test Plan

pull-bot commented Mar 28, 2022 • edited Loading

analysis-bot commented Mar 28, 2022 • edited Loading

analysis-bot commented Mar 29, 2022 • edited Loading

ShikaSD left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 29, 2022

facebook-github-bot commented Apr 1, 2022

ShikaSD Apr 1, 2022

Choose a reason for hiding this comment

ShikaSD Apr 1, 2022

Choose a reason for hiding this comment

kmagiera Apr 4, 2022

Choose a reason for hiding this comment

ShikaSD Apr 4, 2022

Choose a reason for hiding this comment

kmagiera commented Apr 5, 2022

facebook-github-bot commented Apr 20, 2022

react-native-bot commented Apr 21, 2022

facebook-github-bot commented Apr 21, 2022

pull-bot commented Mar 28, 2022 •

edited

Loading

analysis-bot commented Mar 28, 2022 •

edited

Loading

analysis-bot commented Mar 29, 2022 •

edited

Loading