parse-community / ParseLiveQuery-iOS-OSX

Parse LiveQuery Client for iOS/OS X.
https://parseplatform.org
Other
192 stars 133 forks source link

Update Bolts dependency to only use the Tasks subspec. #222

Closed JoeSzymanski closed 4 years ago

JoeSzymanski commented 4 years ago

This removes the import of Bolts/AppLinks by limiting the dependency to only Bolts/Tasks. Bolts/AppLinks includes some UIWebView code that can't be used anymore.

This will need to be updated and released before April 2020, since Apple will stop accepting new application uploads that have any references to UIWebView

noobs2ninjas commented 4 years ago

So we are actually working on a new parse update that we hope to release later this week. One of the improvements is that it uses Parse’s own Bolts fork. Facebook stopped updating their repo. One of the first improvements we made to our fork Is updating to the new WKWebView(Cant recall the name) so that Mac 10.15 and catalyst can build with no errors.

At one point we had discussed switching it to tasks a few times. Unfortunately this only fixes it for cocoapods and not Carthage which is why we went that route instead.

You can still help us out though. Instead of doing this, when the new update drops you can update the parse dependency version. Also, you can update the Bolts to point to parse-community/Bolts-ObjC

JoeSzymanski commented 4 years ago

I think this change is still necessary, even with those coming changes. ParseLiveQuery shouldn't be pulling in Bolts/AppLinks anyway, since it's not required by this code at all. In addition, this change needs to be available before April 1, since Apple is going to start rejecting new apps at that point that use UIWebView in any way. This change will solve the problem in the short term until the new solution is available.

noobs2ninjas commented 4 years ago

You’re going to be pulling in bolts/app-links anyway because its still a dependency of Parse. Not sure what cocoapods will do then. Pull in and load bolts twice? Either way. Considering this isn’t a stand-alone library, our best bet is still to wait for the parse update so we can switch the dependency to our fork since that will be added no matter what on next release.

JoeSzymanski commented 4 years ago

Parse was updated to only include Bolts/Tasks: https://github.com/parse-community/Parse-SDK-iOS-OSX/blob/master/Parse.podspec#L83

That's where I got the idea to change this dependency.

JoeSzymanski commented 4 years ago

As a side note, I've validated that this change will remove the UIWebView code by testing the pod install with our own repos.

drdaz commented 4 years ago

ParseLiveQuery depends on Parse-iOS-SDK which depends on Bolts.

Shouldn't we be good without it here?

EDIT: I haven't tested it, but I can't imagine right now how you can end up in a workspace using PLQ that doesn't already include Bolts, if we're using Cocoapods.

drdaz commented 4 years ago

Hmm... this is why it was added. Looks like it was a case of 'it needs this now'. https://github.com/parse-community/ParseLiveQuery-iOS-OSX/pull/142

EDIT: I think It's worth checking if this works without the explicit dependency again. IIRC both @noobs2ninjas and myself have poked at these dependencies a little, here and in the main SDK.

JoeSzymanski commented 4 years ago

I think we do need the explicit dependency, though I think it needs to be set to "Bolts/Tasks" to remove the dependency on the unnecessary subspec.

noobs2ninjas commented 4 years ago

@JoeSzymanski My bad. I didn't realize we had updated Parse to that. I was under impression that the subspec was decently new. Thats my bad. I do agree with @drdaz I think its worth testing if we remove this line completely since Bolts/Tasks is a dependency of Parse. I think it will still work but no big deal if it doesn't.

noobs2ninjas commented 4 years ago

Actually, assuming it works, I think we have to remove the line. If this leaves it in then it will pull 2 Bolts. One from the source for ParseLiveQuery and the one from our fork for Parse.

JoeSzymanski commented 4 years ago

I did a little more digging into this today:

  1. It appears to be safe to completely remove the exlicit Bolts dependency. My testing builds work fine.
  2. Bolts appears to only be used in this file https://github.com/parse-community/ParseLiveQuery-iOS-OSX/blob/master/Sources/ParseLiveQuery/Internal/BoltsHelpers.swift. However, as far as I can tell, those methods aren't referenced anywhere and probably are safe to remove entirely anyway. They are marked as internal, but are still part of the public interface.

I can get a new pull request up with whatever path we think makes sense to move forward with here.

noobs2ninjas commented 4 years ago

@Jawnnypoo

Perfect. Thanks for taking the time to do that for us.

For right now I think we would be best removing it entirely. Although we are in talks with Facebook right now to get Bolts turned back over to us, currently they aren't maintaining it at all and have ignored all attempts by us to make it Swift 5.0, Catalyst, and Catalina compatible. While we have been updating our own for, that doesn't help us with Cocoapods. So, the less we can rely on that for now, the better.

JoeSzymanski commented 4 years ago

@noobs2ninjas To be clear, do you want to just remove the explicit dependency, or do you want to remove the helper class so that there's zero dependency on Bolts at all?

noobs2ninjas commented 4 years ago

@JoeSzymanski

I'm not sure about the helper class. While not essential, is there for the developer in order to easily convert Task to BFTask and vice versa. This is helpful if you deal with both Swift and Obj-C in your project. Not sure though. What do you think @drdaz?

Edit: For now I'm going to say yes. For reasons that nothing else uses Bolts-ObjC here. So, the only way this would be helpful is if the developer uses bolts in their own project aside from Parse and Parse Live Query all while programming in both Obj-C and Swift. Pretty unlikely. Dont think its warranted to have an extra file just for that specific niche case.

JoeSzymanski commented 4 years ago

Personally, I'd lean towards removing the helper class from here, as it's a relatively simple component for a developer to add to their own code if needed. It feels like there's no need to have it be a required part of this library, when removing it completely removes the Bolts dependency.

noobs2ninjas commented 4 years ago

I agree. You replied too quick before I put my edit on my last message.

JoeSzymanski commented 4 years ago

I'll create a new Pull request removing everything this afternoon. I'll close this once that one is opened

JoeSzymanski commented 4 years ago

New PR is available at https://github.com/parse-community/ParseLiveQuery-iOS-OSX/pull/223