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

Break Circular Dependency between React-Codegen and React-Fabric #36210

Closed
wants to merge 5 commits into from

Conversation

cipolleschi
Copy link
Contributor

Summary:
One of the circular dependencies we have in OSS was between React-Codegen and React-Fabric.

React-Codegen generates component which has to depends on React-Fabric because they need to use the files contained in the react/renderer/view folder.

React-Fabric contains some components that depends on RNCore, which was generated inside the React-Codegen folder.

This change generates the RNCore components inside the ReactCommon/react/renderer/components/rncore folder, breaking the dependency as rncore folder is now contained by React-Fabric itself.

Fun Fact: That's how it always should have been. There was already a line in the .gitignore to exclude the content of ReactCommon/react/renderer/components/rncore folder. I guess that with some of the refactoring/previous projects on Codegen, this requirements has slipped.

Changelog:

[iOS][Breaking] - generates RNCore components inside the ReactCommon folder

Differential Revision: D43304641

cipolleschi and others added 5 commits February 16, 2023 08:30
…oid name clashes

Summary:
Change the `React-RCTFabric` prefix and `module_name` from React to `RCTFabric`.

In OSS, React-Core and React-RCTFabric are two different podspec, which results in two different frameworks. We can't have two frameworks with the same `module_name` as this is a naming clash, therefore I had to rename the `React-RCTFabric` `module_name` from `React` to `CTFabric.

This change affected also Codegen which needed to be updated to reflect the new framework namespace.

## Changelog:
[iOS][Changed] - Change module_name and prefix of `React-RCTFabric` from `React` to `RCTFabric`

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

fbshipit-source-id: a3253a9c75179e05068cb18ac1cbfc659024c8f2
Summary:
This change solve a Circular Dependency where
- `React-Core` depends on `ReactCommon` because `RCTAppSetupUtils.h` (in Core) imports the `RCTTurboModuleManager` (from ReactCommon)
- `RCTTurboModuleManager` in `ReactCommon` depends on `React-Core` because it imports several classes from it (e.g. the `RCTBridge` class)

## Changelog:
[iOS][Changed] - Moved the RCTAppSetupUtils to the AppDelegate library to break a dependency cycle

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

fbshipit-source-id: f67e8f913ee8baafcba35f1d6cc761301c264a14
Summary: Update podspecs with the right search paths to include the required framework by every module.

Differential Revision: D43089372

fbshipit-source-id: bb1dd56cadc3055b45f890ab72219948904aa9a1
Summary:
This diff update the Cocoapods scripts to install the proper dependencies and search paths when `use_frameworks!` is setup.

It also adds unit tests for the changes and the new methods introduced.

## Changelog:
[iOS][Changed] - Properly install dependencies with `use_frameworks!`

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

fbshipit-source-id: bd9df426f9facd7649f3ad655b86aa8edb17fc93
Summary:
One of the circular dependencies we have in OSS was between React-Codegen and React-Fabric.

React-Codegen generates component which has to depends on React-Fabric because they need to use the files contained in the `react/renderer/view` folder.

React-Fabric contains some components that depends on RNCore, which was generated inside the React-Codegen folder.

This change generates the RNCore components inside the `ReactCommon/react/renderer/components/rncore` folder, breaking the dependency as `rncore` folder is now contained by React-Fabric itself.

**Fun Fact:** That's how it always should have been. There was already a line in the `.gitignore` to exclude the content of `ReactCommon/react/renderer/components/rncore` folder. I guess that with some of the refactoring/previous projects on Codegen, this requirements has slipped.

## Changelog:
[iOS][Breaking] -  generates RNCore components inside the ReactCommon folder

Differential Revision: D43304641

fbshipit-source-id: 94426ed10a3869c9e03c9a144dc0785cc13c0dfe
@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 19, 2023
@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,451,953 +0
android hermes armeabi-v7a 7,775,333 +0
android hermes x86 8,927,562 +0
android hermes x86_64 8,785,165 +0
android jsc arm64-v8a 6,668,753 +0
android jsc armeabi-v7a 7,462,720 +0
android jsc x86 9,192,880 +0
android jsc x86_64 6,893,882 +0

Base commit: 421df9f
Branch: main

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Feb 20, 2023
…ebook#36210)

Summary:
Pull Request resolved: facebook#36210

One of the circular dependencies we have in OSS was between React-Codegen and React-Fabric.

React-Codegen generates component which has to depends on React-Fabric because they need to use the files contained in the `react/renderer/view` folder.

React-Fabric contains some components that depends on RNCore, which was generated inside the React-Codegen folder.

This change generates the RNCore components inside the `ReactCommon/react/renderer/components/rncore` folder, breaking the dependency as `rncore` folder is now contained by React-Fabric itself.

**Fun Fact:** That's how it always should have been. There was already a line in the `.gitignore` to exclude the content of `ReactCommon/react/renderer/components/rncore` folder. I guess that with some of the refactoring/previous projects on Codegen, this requirements has slipped.

## Changelog:
[iOS][Breaking] -  generates RNCore components inside the ReactCommon folder and create a new pod for platform-specific ImageManager classes

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

fbshipit-source-id: 327a80c15e1fd77d387d496a51b037c690a9867f
cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Feb 20, 2023
…ebook#36210)

Summary:
Pull Request resolved: facebook#36210

One of the circular dependencies we have in OSS was between React-Codegen and React-Fabric.

React-Codegen generates component which has to depends on React-Fabric because they need to use the files contained in the `react/renderer/view` folder.

React-Fabric contains some components that depends on RNCore, which was generated inside the React-Codegen folder.

This change generates the RNCore components inside the `ReactCommon/react/renderer/components/rncore` folder, breaking the dependency as `rncore` folder is now contained by React-Fabric itself.

**Fun Fact:** That's how it always should have been. There was already a line in the `.gitignore` to exclude the content of `ReactCommon/react/renderer/components/rncore` folder. I guess that with some of the refactoring/previous projects on Codegen, this requirements has slipped.

## Changelog:
[iOS][Breaking] -  generates RNCore components inside the ReactCommon folder and create a new pod for platform-specific ImageManager classes

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

fbshipit-source-id: 4d21857cef67c373475ecea75f53a6f8f869fbde
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 20, 2023
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 5d175c6.

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…ebook#36210)

Summary:
Pull Request resolved: facebook#36210

One of the circular dependencies we have in OSS was between React-Codegen and React-Fabric.

React-Codegen generates component which has to depends on React-Fabric because they need to use the files contained in the `react/renderer/view` folder.

React-Fabric contains some components that depends on RNCore, which was generated inside the React-Codegen folder.

This change generates the RNCore components inside the `ReactCommon/react/renderer/components/rncore` folder, breaking the dependency as `rncore` folder is now contained by React-Fabric itself.

**Fun Fact:** That's how it always should have been. There was already a line in the `.gitignore` to exclude the content of `ReactCommon/react/renderer/components/rncore` folder. I guess that with some of the refactoring/previous projects on Codegen, this requirements has slipped.

## Changelog:
[iOS][Breaking] -  generates RNCore components inside the ReactCommon folder and create a new pod for platform-specific ImageManager classes

Reviewed By: sammy-SC, dmytrorykun

Differential Revision: D43304641

fbshipit-source-id: ebb5033ce73dbcd03f880c3e204511fdce04b816
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 Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants