Skip to content

refactor: use install_modules_dependencies method only if react native version >=0.71.0 #358

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

Conversation

ykhateeb
Copy link
Contributor

This pull request replaces all the inline IOS dependencies in podspec and uses install_modules_dependencies method to depend on React Native core changes.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. Can you provide some more details about this method like what does this do, why was this added, which version of React Native does this support etc.?

@ykhateeb
Copy link
Contributor Author

Hey @satya164,

This method contains all the needed IOS dependencies (same as the current that we have now) with supporting old and new architecture, the purpose of adding it is to simplify the process of migrating to a new React Native version, by depending on React Native changes rather than duplicate same code, this method exists starting from v0.71.0.

Here is the full implementation: https://github.com/facebook/react-native/blob/febf6b7f33fdb4904669f99d795eba4c0f95d7bf/scripts/cocoapods/new_architecture.rb#L79

@ykhateeb ykhateeb requested a review from satya164 February 26, 2023 16:42
Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ykhateeb thanks. I don't want to drop support for older RN versions yet until 0.71 is a few releases old. Can you wrap this in the condition so that this is only used for new architecture?

@ykhateeb ykhateeb force-pushed the use-install_modules_dependencies-in-ios-podspec branch from 44bf0d9 to 380e8ea Compare February 27, 2023 16:25
@ykhateeb
Copy link
Contributor Author

ykhateeb commented Feb 27, 2023

@satya164 Could you please check it now?

@ykhateeb ykhateeb requested a review from satya164 February 27, 2023 16:27
@ykhateeb ykhateeb changed the title Replace inline IOS dependencies with install_modules_dependencies method Use install_modules_dependencies only if react native version >0.71.0 Feb 27, 2023
@ykhateeb ykhateeb changed the title Use install_modules_dependencies only if react native version >0.71.0 Use install_modules_dependencies method only if react native version >0.71.0 Feb 27, 2023
@ykhateeb ykhateeb changed the title Use install_modules_dependencies method only if react native version >0.71.0 Use install_modules_dependencies method only if react native version >=0.71.0 Feb 27, 2023
@ykhateeb ykhateeb changed the title Use install_modules_dependencies method only if react native version >=0.71.0 feat: use install_modules_dependencies method only if react native version >=0.71.0 Feb 28, 2023
@satya164 satya164 force-pushed the main branch 6 times, most recently from 99cfbff to 0775dc9 Compare March 10, 2023 15:30
@ykhateeb ykhateeb changed the title feat: use install_modules_dependencies method only if react native version >=0.71.0 refactor: use install_modules_dependencies method only if react native version >=0.71.0 Mar 15, 2023
Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@satya164 satya164 enabled auto-merge (squash) April 3, 2023 06:43
@satya164 satya164 merged commit fca84d8 into callstack:main Apr 3, 2023
satya164 pushed a commit that referenced this pull request May 16, 2024
<!-- Please provide enough information so that others can review your
pull request. -->
<!-- Keep pull requests small and focused on a single change. -->

### Summary

<!-- What existing problem does the pull request solve? Can you solve
the issue with a different approach? -->

After #358
identation become shifted which makes code hard to read and could lead
to bugs. For example I was placing package dependencies inside `if/else`
block, right after `s.dependency "React-Core"`

see 
| before | after |
|--------|--------|
| <img width="1624" alt="image"
src="https://github.com/callstack/react-native-builder-bob/assets/1577804/65a6b479-3e76-4170-b874-b5635b6a5401">
| <img width="1624" alt="image"
src="https://github.com/callstack/react-native-builder-bob/assets/1577804/5e3862e7-e6d9-48da-9efd-5c3e5f611749">
|



### Test plan

<!-- List the steps with which we can test this change. Provide
screenshots if this changes anything visual. -->

Purely aesthetic changes, so nothing to break
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants