-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
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
This pull request was exported from Phabricator. Differential Revision: D43304641 |
Base commit: 421df9f |
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 asrncore
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 ofReactCommon/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