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

[RN][Fabric] Most library got broken after breaking change on ComponentDescriptor::adopt signature #41699

Closed
cipolleschi opened this issue Nov 29, 2023 · 4 comments
Assignees
Labels
Needs: React Native Team Attention Tech: Fabric Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@cipolleschi
Copy link
Contributor

Description

The Problem

The 8th of September, this commit landed on main. This happened before the 0.73 branch cut, therefore it will affect all the apps that are going to be created with 0.73.

The commit is technically correct: before the commit, the ComponentDescriptor could take ownership of the ShadowNode that was passed in. Semantically, this is incorrect for Fabric, as the ComponentDescriptor should not own the reference to the ShadowNode.

The commit changes this behavior and prevent this possibility from happening.

To achieve this, the signature of the adopt method has changed from

void adopt(const ShadowNode::Unshared& shadowNode) const override

to

void adopt(ShadowNode& shadowNode) const override

This was a breaking change for all the libraries already migrated to Fabric, but it was marked as in [Internal] change.

Mitigation

We still want for this change to happen in the stable, final version of Fabric.

However, we want to give some time to library maintainers to migrate away from the old signature to the new one.

To do so, another PR has been created and it is going to be picked in the latest RC, so that library that have been migrated already to Fabric will continue working.

We are going to remove the old signature in ReactNative 0.74

Call to Action

If you are a library maintainer of a library that already migrated to Fabric, please:

  1. Check if your component was overriding the adopt method (not all the components have to do it)
  2. If yes, please migrate away from the old signature toward the new one.

React Native Version

0.73.0-rc.0

Output of npx react-native info

Not Relevant

Steps to reproduce

npx react-native@next init BrokenLib --version @next --skip-install
cd BrokenLib
yarn add react-native-svg # or any other library already migrated to Fabric
yarn install
cd ios
bundle install
RCT_NEW_ARCH_ENABLED=1 bundle exec pod install
open BrokenLib.xcworkspace

In Xcode: + B to build the app

Snack, screenshot, or link to a repository

Screenshot 2023-11-29 at 10 13 52
@cipolleschi cipolleschi added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Nov 29, 2023
@github-actions github-actions bot added the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Nov 29, 2023
Copy link

⚠️ Missing Reproducible Example
ℹ️ We could not detect a reproducible example in your issue report. Please provide either:
  • If your bug is UI related: a Snack
  • If your bug is build/update related: use our Reproducer Template. A reproducer needs to be in a GitHub repository under your username.

@cipolleschi cipolleschi added Tech: Fabric and removed Needs: Triage 🔍 Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. labels Nov 29, 2023
@facebook facebook locked and limited conversation to collaborators Nov 29, 2023
@fabOnReact
Copy link
Contributor

How can I help with this task? Thanks a lot

@cortinico
Copy link
Contributor

How can I help with this task? Thanks a lot

Hi @fabriziobertoglio1987
@cipolleschi and @sammy-SC already worked on this task. We've updated the code to be backward compatible for existing libraries

@cortinico
Copy link
Contributor

Closing as the breaking change got mitigated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs: React Native Team Attention Tech: Fabric Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

4 participants