search

oblador / react-native-vector-icons

Customizable Icons for React Native with support for image source and full styling.
https://oblador.github.io/react-native-vector-icons/
MIT License
17.31k stars 2.12k forks source link

fix: proper usage of turbomodule #1550

Closed WoLewicki closed 8 months ago

WoLewicki commented 9 months ago

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

WoLewicki commented 9 months ago

@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 commented 8 months ago

@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.

WoLewicki commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.

oblador commented 8 months ago

Released in 10.0.1

Sunhat commented 7 months ago

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 commented 7 months ago

@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?