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

Target iOS 12.0 #786

Merged
merged 1 commit into from Jul 11, 2023
Merged

Target iOS 12.0 #786

merged 1 commit into from Jul 11, 2023

Conversation

hsjoberg
Copy link
Contributor

@hsjoberg hsjoberg commented Jul 1, 2023

Summary

Hey.
value which is available in iOS >= 12.0 is being used, causing compilation to fail on newer XCode versions.

image

This PR bumps the iOS target to version 12.0.

I'm not sure about tvOS.

Checklist

  • I have tested this on a device and a simulator

@hsjoberg hsjoberg requested a review from zoontek as a code owner July 1, 2023 15:44
@mikehardy
Copy link

That's a compilation error from RCTTypeSafety, which is a react library.
I believe the proposed patch here is invalid then, since it is not related to this library.

Please search first (it shows up in the first results on a web search as a react-native issue already fixed in later versions - facebook/react-native#34106

@hsjoberg
Copy link
Contributor Author

hsjoberg commented Jul 2, 2023

@mikehardy This issue is affecting multiple libs across the ecosystem. For example react-native-svg: software-mansion/react-native-svg#2041

Yes, I'm aware of the GitHub issue about react_native_pods.rb -- I am already running react-native 0.72.1 (this is from a fresh installation).

Furthermore, it looks like RCTTypeSafety sets 12.4 as the iOS version.
https://github.com/facebook/react-native/blob/v0.72.1/packages/react-native/Libraries/TypeSafety/RCTTypeSafety.podspec#L26

So I still believe this PR is valid, although I agree it's a bit weird.
You can try for yourself with XCode 14.3.

@mikehardy
Copy link

This PR is a breaking change that causes this library to no longer support older versions, and is related to code that is not in the library.

It is off-target here, I do not believe it is valid.

Use a Podfile post_install step to modify the target versions if you need it, I think.

@mikehardy
Copy link

I verified this would drop support for anyone on react-native < 0.69, 0.68 supported iOS11
facebook/react-native@c71e6ef

This may be something @zoontek agrees with, it may not, but it seems pretty strange for me for a react-native issue to force libraries to adopt a breaking change that cuts off older versions - this is part of what forces people to prematurely landfill devices for no good reason

RNPermissions.podspec Outdated Show resolved Hide resolved
@hsjoberg
Copy link
Contributor Author

hsjoberg commented Jul 4, 2023

I've updated the PR as per @numandev1's suggestion.

@zoontek
Copy link
Owner

zoontek commented Jul 7, 2023

@hsjoberg Hi, and thanks for the PR! I have a small suggestion: maybe we should migrate to install_modules_dependencies too (to fit what I do in my others libs podspec), like this:

require "json"

package = JSON.parse(File.read(File.join(__dir__, "package.json")))

Pod::Spec.new do |s|
  s.name         = "RNPermissions"

  s.version      = package["version"]
  s.license      = package["license"]
  s.summary      = package["description"]
  s.author       = package["author"]
  s.homepage     = package["homepage"]

  s.requires_arc = true

  s.source       = { :git => package["repository"]["url"], :tag => s.version }
  s.source_files = "ios/*.{h,m,mm}"

  if ENV['RCT_NEW_ARCH_ENABLED'] == '1' then
    install_modules_dependencies(s)
    s.platforms  = { :ios => "12.4", :tvos => "12.4" }
  else
    s.dependency   "React-Core"
    s.platforms  = { :ios => "10.0", :tvos => "11.0" }
  end
end

This solve the issue without any breaking and its IMHO a cleaner way to handle both archs. Could you make the change and copy this instead?
PS: We target 12.4 since that's the version targeted by react-native since v0.69.0

@hsjoberg
Copy link
Contributor Author

@zoontek sure I'll fix that.

@hsjoberg
Copy link
Contributor Author

hsjoberg commented Jul 10, 2023

I've changed to suggested code and tested building with new arch both activated and inactivated.

@zoontek zoontek merged commit 9ff2bf5 into zoontek:master Jul 11, 2023
@zoontek
Copy link
Owner

zoontek commented Jul 11, 2023

Thanks, it's published under 3.8.4: https://github.com/zoontek/react-native-permissions/releases/tag/3.8.4

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.

None yet

4 participants