-
Notifications
You must be signed in to change notification settings - Fork 361
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
Remove PIN_APP_EXTENSION requirement #68
Remove PIN_APP_EXTENSION requirement #68
Conversation
.travis.yaml now also includes |
Also, I couldn't find a way to do this in a way that wouldn't break existing internal behavior, so this would have to bump the version to 3.0 |
|
||
#if !TARGET_OS_WATCH | ||
#if __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_4_0 | ||
#import <UIKit/UIKit.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is UIKit not available on watchOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this looks like the 2 are related, but actually a lot of things in UIApplication.h aren't available, so I just wrapped the whole class in this.
I should have looked at comments first. Can you explain what is broken with existing internal behavior? |
Right now, if you want to use PINCache with an extension, you compile it with a macro, and that just compiles out the parts that don't work in the extensions, and then your app and your extension can share this framework. The big downside to this is that your app can no longer use the background task behavior, since it's been compiled out. In the ideal case, you have one framework which can be used by both the extension and the app, and when your app uses it, it can still take advantage of the background task behavior. To achieve this, you have to have all methods that want to use APIs that are not available in extensions marked as NS_EXTENSION_UNAVAILABLE. Because PINDiskCache might start creating UIApplication background tasks after init, you have to decide whether or not PINDiskCache can do this by either defining a new initializer that's marked as NS_EXTENSION_UNAVAILABLE or marking the existing initializer as NS_EXTENSION_UNAVAILABLE. When you make this decision, either one client or another is going to lose out: either previous extension clients of PINDiskCache are going to have to start calling a new extension-compatible initializer, or application clients of PINDiskCache are going to start having to call a new initializer that is marked as NS_EXTENSION_UNAVAILABLE (if they wanna take advantage of the behavior that's only available in apps). And unfortunately, this decision will also cascade up into PINCache.h as well. |
It lints! |
if (self = [super init]) { | ||
static dispatch_once_t onceToken; | ||
dispatch_once(&onceToken, ^{ | ||
gBackgroundTaskClass = [PINApplicationBackgroundTask class]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to switch to using an ivar, since calling this initializer currently permanently alters how all PINDiskCaches behave after this call
I think I understand :) Based on what you've said it sounds like we need to have a default behavior for PINDiskCache and by proxy PINCache. Which do you think is better, background support by default or not? My feeling is it should probably use the background support APIs by default which will cause a hassle for extensions, but it will break as opposed to just not handle things correctly silently. |
@benasher44 I'll try and look at this in depth tomorrow or this weekend. Are y'all blocked on this? Will getting it merged by then work? |
@garrettmoon We're more blocked on PINCache 2.2, and hopefully #69 will get that all sorted out. This is something we definitely want soon in the coming weeks, but it's not as pressing for us. Thanks for all of the consideration :) |
226f55f
to
3d709ca
Compare
@garrettmoon okay yeah so if I go that route, if most users don't have extensions (prob the case), then the upgrade is easy. For users with extensions, they'll get a compiler error. This makes sense. I'll update later today |
@garrettmoon In that case, do you think extensions should have an extension-safe variants of sharedCache singletons? |
@benasher44 Yeah, that seems like a good call to me. |
for (NSURL *trashedItemURL in trashedItems) { | ||
NSError *removeTrashedItemError = nil; | ||
[[NSFileManager defaultManager] removeItemAtURL:trashedItemURL error:&removeTrashedItemError]; | ||
PINDiskCacheError(removeTrashedItemError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation here seems to have changed from 4-space to 2-space, might want to re-indent for consistency.
62aa737
to
88b0d28
Compare
@benasher44 the other concern I had after seeing this (facebookarchive/AsyncDisplayKit#1309) is that adopting this solution will require separate APIs for projects which rely on PINCache as well. Are you opposed to the .appex solution? @maicki put up a PR here: #72 I think we'd also want to at least add a extension target and unit test to make sure it doesn't break in the future. |
@garrettmoon @maicki so my other suspicion was that this wouldn't work when PINCache is built as a framework, but I just tested that with no issues. Ship it (with tests and an extension target please)! |
For future reference, this is my preferred method for checking if the currently running process is an NSExtension which does't rely on the underlying file extension. I think I first learned this trick here under "AmIRunningAsAnExtension". Take it or leave it :)
|
This PR is might still be a bit rough around the edges, but everything now pod lib lints, and it also works in an extension without having to compile out the UIApplication background task bits. I could however get the
__WATCHOS_UNAVAILABLE
macro to work in the same way, so I just decided to screw that and compileTARGET_OS_WATCH
incompatible bits out of the code.