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

Lower deployment target #16

Closed
kambala-decapitator opened this issue Jun 4, 2021 · 9 comments · Fixed by #19
Closed

Lower deployment target #16

kambala-decapitator opened this issue Jun 4, 2021 · 9 comments · Fixed by #19

Comments

@kambala-decapitator
Copy link

Not sure if it should be treated as a feature or bug.

Have you read the Contributing Guidelines?
Yes.

Describe the feature

Lower deployment targets.

Is your feature request related to a problem?

Yes, I can't depend on this library because my library supports lower deployment target.

Proposed solution

Simply lower them.

Alternatives considered

None.

Additional context

For example, lint succeeds with setting iOS to 9.0:

❯ bundle exec pod lib lint --platforms=ios

 -> Foil (1.1.0)
    - NOTE  | xcodebuild:  note: Using new build system
    - NOTE  | xcodebuild:  note: Building targets in parallel
    - NOTE  | xcodebuild:  note: Using codesigning identity override: -
    - NOTE  | [iOS] xcodebuild:  note: Planning build
    - NOTE  | [iOS] xcodebuild:  note: Constructing build description
    - NOTE  | [iOS] xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')

Foil passed validation.
@basememara
Copy link
Collaborator

Property Wrappers was released with iOS 12 / Swift 5.1. I'm not sure if it's possible to run it on older versions.

@kambala-decapitator
Copy link
Author

Swift libs are copied to device, of course it's possible.

For verification I've successfully launched demo app on device running iOS 10. Can also do that on iOS 9 later if needed.

@basememara
Copy link
Collaborator

That's a really old version to commit to maintaining tbh 😬 altho I feel your pain. How about opening up a PR that you can consume so we can also see what cascading effects it has with all the tooling and tests we have around the package?

@kambala-decapitator
Copy link
Author

opened #17

@jessesquires
Copy link
Owner

Hey, thanks @kambala-decapitator 😄

I agree with @basememara's hesitation — iOS 9 is quite old, and I am not willing to officially commit to supporting it.

I think a better solution for you would be to implement a post-install hook in your Podfile that resets the deployment target for Foil.

An example:

post_install do |installer|
  installer.pods_project.targets.each do |target|
    target.build_configurations.each do |config|
      config.build_settings['IPHONEOS_DEPLOYMENT_TARGET'] = '9.0'
    end
  end
end

This resets for all pods, but you could modify to check specifically for Foil.

@kambala-decapitator
Copy link
Author

kambala-decapitator commented Jun 5, 2021

Thanks @jessesquires, I know about such workaround. But I'm working on a library and wanted to add Foil as dependency. The only way to make that work would be maintaining my own fork with lower deployment target(s) then.

Not quite sure what is blocking you here: deployment target really matters when using SDK features, but here you're basically using only Swift language features.

@jessesquires
Copy link
Owner

Not quite sure what is blocking you here

Generally speaking: my concern is simply committing to such a low deployment target and how that affects maintenance. In the future, we may want to take advantage of some iOS 12+ feature. Then, it could be necessary to drop iOS 10 and below, which is a breaking change and not ideal.

For this library specifically: I doubt this would happen. This library is small and unlikely to change much.

but here you're basically using only Swift language features.

And this is a very good point. I think I'm on-board with lowering the targets. I'll also fix #18 -- thank you for that!

@jessesquires jessesquires added this to the NEXT milestone Jun 5, 2021
jessesquires added a commit that referenced this issue Jun 5, 2021
- Lower iOS and tvOS to 9.0, closes #16
- Correct SPM platforms and make consistent with podspec, closes #18

This also separates tests into `ObservationTests`, which we can mark as `@available` so that building/running via SPM works.
jessesquires added a commit that referenced this issue Jun 6, 2021
- Lower iOS and tvOS to 9.0, closes #16
- Correct SPM platforms and make consistent with podspec, closes #18

This also separates tests into `ObservationTests`, which we can mark as `@available` so that building/running via SPM works.
@jessesquires
Copy link
Owner

@kambala-decapitator Thanks again for opening this issue and discussing with us. 😄

I just tagged a new release and pushed to cocoapods. https://github.com/jessesquires/Foil/releases/tag/1.2.0

(also includes #18 )

@kambala-decapitator
Copy link
Author

Thank you very much! :)

Generally speaking: my concern is simply committing to such a low deployment target and how that affects maintenance.

In general I fully agree with you! But when it's simple or even trivial to support older devices, I don't see a reason not to do so :)

In the future, we may want to take advantage of some iOS 12+ feature. Then, it could be necessary to drop iOS 10 and below, which is a breaking change and not ideal.

That's completely fine: at some point of development life cycle requirements and goals may change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants