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

Implement public instances for text nodes in Fabric #26516

Merged

Conversation

rubennorte
Copy link
Contributor

@rubennorte rubennorte commented Mar 30, 2023

Summary

This adds the ability to create public instances for text nodes in Fabric. The implementation for the public instances lives in React Native (as it does for host components after #26437). The logic here just handles their lazy instantiation when requested via getPublicInstanceFromInternalInstanceHandle, which is called by Fabric with information coming from the shadow tree.

It's important that the creation of public instances for text nodes is done lazily to avoid regressing memory usage when unused. Instances for text nodes are left intact if the public instance is never accessed.

This is necessary to implement access to text nodes in React Native as explained in react-native-community/discussions-and-proposals#607

How did you test this change?

Added unit tests (also fixed a test that was only testing the logic in a mock :S).

@react-sizebot
Copy link

react-sizebot commented Mar 30, 2023

Comparing: 29a3be7...c9ac0e3

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 = 162.20 kB 162.20 kB = 51.29 kB 51.29 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 163.85 kB 163.85 kB = 51.79 kB 51.79 kB
facebook-www/ReactDOM-prod.classic.js = 556.25 kB 556.25 kB = 98.33 kB 98.33 kB
facebook-www/ReactDOM-prod.modern.js = 539.97 kB 539.97 kB = 95.88 kB 95.88 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/shims/ReactNativeTypes.js +0.68% 7.98 kB 8.04 kB +0.42% 2.16 kB 2.17 kB

Generated by 🚫 dangerJS against c9ac0e3

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 4, 2023
Summary:
This exposes a new method in the private interface used by React so we can merge facebook/react#26516

We're adding support for text instances in React Native (as defined in react-native-community/discussions-and-proposals#607). See D44632362 for the full implementation.

Changelog: [internal]

bypass-github-export-checks

Reviewed By: sammy-SC

Differential Revision: D44663223

fbshipit-source-id: 70ca3ca9d2edefaa73a396f43c2d560c6d1422f1
@rubennorte rubennorte merged commit 0700dd5 into facebook:main Apr 4, 2023
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 13, 2023
Summary:
This sync includes the following changes:
- **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([#26599](facebook/react#26599)) //<Andrew Clark>//
- **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([#26597](facebook/react#26597)) //<Andrew Clark>//
- **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent  ([#26594](facebook/react#26594)) //<Sebastian Markbåge>//
- **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([#26583](facebook/react#26583)) //<Sebastian Markbåge>//
- **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([#26593](facebook/react#26593)) //<Josh Story>//
- **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([#26592](facebook/react#26592)) //<Josh Story>//
- **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([#26591](facebook/react#26591)) //<Josh Story>//
- **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([#26590](facebook/react#26590)) //<Josh Story>//
- **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([#26589](facebook/react#26589)) //<Josh Story>//
- **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([#26552](facebook/react#26552)) //<Andrew Clark>//
- **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([#26580](facebook/react#26580)) //<Sebastian Markbåge>//
- **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([#26573](facebook/react#26573)) //<Sebastian Markbåge>//
- **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([#26568](facebook/react#26568)) //<Josh Story>//
- **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([#26561](facebook/react#26561)) //<Andrew Clark>//
- **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([#26558](facebook/react#26558)) //<Sebastian Markbåge>//
- **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([#26553](facebook/react#26553)) //<Sophie Alpert>//
- **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([#26551](facebook/react#26551)) //<Sebastian Markbåge>//
- **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([#26534](facebook/react#26534)) //<Mohammad Ghorbani>//
- **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([#26546](facebook/react#26546)) //<Sebastian Markbåge>//
- **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([#26236](facebook/react#26236))" ([#26528](facebook/react#26528)) //<Jan Kassens>//
- **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([#26516](facebook/react#26516)) //<Rubén Norte>//
- **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([#26526](facebook/react#26526)) //<Josh Story>//
- **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([#26535](facebook/react#26535)) //<Hans Otto Wirtz>//
- **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([#26532](facebook/react#26532)) //<Andrew Clark>//
- **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([#26531](facebook/react#26531)) //<Andrew Clark>//
- **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([#26512](facebook/react#26512)) //<Andrew Clark>//
- **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([#26521](facebook/react#26521)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions ca01f35...58742c2

jest_e2e[run_all_tests]
bypass-github-export-checks

Reviewed By: sammy-SC

Differential Revision: D44872333

fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
kassens pushed a commit to kassens/react that referenced this pull request Apr 17, 2023
## Summary

This adds the ability to create public instances for text nodes in
Fabric. The implementation for the public instances lives in React
Native (as it does for host components after facebook#26437). The logic here
just handles their lazy instantiation when requested via
`getPublicInstanceFromInternalInstanceHandle`, which is called by Fabric
with information coming from the shadow tree.

It's important that the creation of public instances for text nodes is
done lazily to avoid regressing memory usage when unused. Instances for
text nodes are left intact if the public instance is never accessed.

This is necessary to implement access to text nodes in React Native as
explained in
react-native-community/discussions-and-proposals#607

## How did you test this change?

Added unit tests (also fixed a test that was only testing the logic in a
mock :S).
jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
Summary:
This exposes a new method in the private interface used by React so we can merge facebook/react#26516

We're adding support for text instances in React Native (as defined in react-native-community/discussions-and-proposals#607). See D44632362 for the full implementation.

Changelog: [internal]

bypass-github-export-checks

Reviewed By: sammy-SC

Differential Revision: D44663223

fbshipit-source-id: 70ca3ca9d2edefaa73a396f43c2d560c6d1422f1
jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
Summary:
This sync includes the following changes:
- **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>//
- **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>//
- **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent  ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>//
- **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>//
- **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>//
- **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>//
- **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>//
- **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>//
- **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>//
- **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>//
- **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>//
- **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>//
- **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>//
- **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>//
- **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>//
- **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>//
- **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>//
- **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>//
- **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>//
- **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>//
- **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>//
- **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>//
- **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>//
- **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>//
- **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>//
- **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>//
- **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions ca01f35...58742c2

jest_e2e[run_all_tests]
bypass-github-export-checks

Reviewed By: sammy-SC

Differential Revision: D44872333

fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This exposes a new method in the private interface used by React so we can merge facebook/react#26516

We're adding support for text instances in React Native (as defined in react-native-community/discussions-and-proposals#607). See D44632362 for the full implementation.

Changelog: [internal]

bypass-github-export-checks

Reviewed By: sammy-SC

Differential Revision: D44663223

fbshipit-source-id: 70ca3ca9d2edefaa73a396f43c2d560c6d1422f1
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>//
- **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>//
- **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent  ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>//
- **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>//
- **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>//
- **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>//
- **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>//
- **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>//
- **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>//
- **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>//
- **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>//
- **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>//
- **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>//
- **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>//
- **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>//
- **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>//
- **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>//
- **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>//
- **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>//
- **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>//
- **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>//
- **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>//
- **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>//
- **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>//
- **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>//
- **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>//
- **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions ca01f35...58742c2

jest_e2e[run_all_tests]
bypass-github-export-checks

Reviewed By: sammy-SC

Differential Revision: D44872333

fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
## Summary

This adds the ability to create public instances for text nodes in
Fabric. The implementation for the public instances lives in React
Native (as it does for host components after facebook#26437). The logic here
just handles their lazy instantiation when requested via
`getPublicInstanceFromInternalInstanceHandle`, which is called by Fabric
with information coming from the shadow tree.

It's important that the creation of public instances for text nodes is
done lazily to avoid regressing memory usage when unused. Instances for
text nodes are left intact if the public instance is never accessed.

This is necessary to implement access to text nodes in React Native as
explained in
react-native-community/discussions-and-proposals#607

## How did you test this change?

Added unit tests (also fixed a test that was only testing the logic in a
mock :S).
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
## Summary

This adds the ability to create public instances for text nodes in
Fabric. The implementation for the public instances lives in React
Native (as it does for host components after #26437). The logic here
just handles their lazy instantiation when requested via
`getPublicInstanceFromInternalInstanceHandle`, which is called by Fabric
with information coming from the shadow tree.

It's important that the creation of public instances for text nodes is
done lazily to avoid regressing memory usage when unused. Instances for
text nodes are left intact if the public instance is never accessed.

This is necessary to implement access to text nodes in React Native as
explained in
react-native-community/discussions-and-proposals#607

## How did you test this change?

Added unit tests (also fixed a test that was only testing the logic in a
mock :S).

DiffTrain build for commit 0700dd5.
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