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

fix: proper usage of turbomodule #1550

Merged
merged 5 commits into from Oct 26, 2023

Conversation

WoLewicki
Copy link
Contributor

PR adding proper usage of TurboModule on iOS with the changes needed for the newest version of RN.

@WoLewicki
Copy link
Contributor Author

@oblador it would be nice if you could check it in free time. Based on how the TurboModule is done in other libs such as react-native-svg. For some reason codegen does not accept color type in spec for modules, so I had to parse the color on the native side. It probably won't work for PlatformColor unfortunately.

@johnf
Copy link
Collaborator

johnf commented Oct 8, 2023

@oblador Can you take a look at this PR

This will be a breaking change for versions of React Native before 0.71.
I had a play with trying to get something to work that is more backwards compatible but am not an iOS dev so didn't have much luck.

I suggest we adopt the same policy as React Native and only support the current version of RN, plus two previous versions.
0.73 is already in beta so that would make this patch in line with the policy.
I can include a table in the README that indicates which versions of RN are supported by which versions of RNVI.

johnf
johnf previously approved these changes Oct 8, 2023
@johnf johnf dismissed their stale review October 8, 2023 03:07

Double checking I think I broke something

@WoLewicki
Copy link
Contributor Author

This will be a breaking change for versions of React Native before 0.71.

Why does it have to be? Or do you mean only new arch?

@johnf
Copy link
Collaborator

johnf commented Oct 9, 2023

This will be a breaking change for versions of React Native before 0.71.

Why does it have to be? Or do you mean only new arch?

install_modules_dependencies only exists from RN 0.71 onwards.

More work could be done to make the podspec more flexible. e.g. put install_modules_dependencies inside the new_arch check and put the old deps back in an else. But it's a lot messier.

I like the approach of using install_modules_dependencies it means we let RN care about what's needed. But it does come at the cost of the breaking change.

@WoLewicki
Copy link
Contributor Author

e.g. put install_modules_dependencies inside the new_arch check and put the old deps back in an else

Yeah I made it like this to keep only the new arch changes as breaking and I am doing it in all libs we migrate. I think it is the best way since you want to change as little of the code as you can so people using old arch can still use it in the same way.

RNVectorIcons.podspec Outdated Show resolved Hide resolved
@oblador oblador merged commit f4e1c1e into oblador:master Oct 26, 2023
1 check passed
@oblador
Copy link
Owner

oblador commented Oct 26, 2023

Released in 10.0.1

@Sunhat
Copy link

Sunhat commented Nov 16, 2023

There appears to be a discussion around this causing a breaking change in older versions of React Native. Some here have said 0.71.0 introduces this new method used, but from what I can tell it's 0.72.0 - Either way this is a breaking change, surely this should've triggered a 11.0.0 release?

From the discussions you've had, there does appear to be some confusion / miscommunication.

@Sunhat
Copy link

Sunhat commented Nov 25, 2023

@oblador 10.0.1 patch introduces a significant breaking change, and yourself/your team are unaware for over a month, unresponsive for over a week?

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.

None yet

4 participants