-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Evolution? #3
Comments
Hi @pranjalsatija ! Thanks for the interest! The guidelines are quite simple, I want to make the SDK as less intrusive as possible, as you may have noticed, pretty much every functionality is coded as protocols. I want to leverage as much as possible the Codable type introduced in Swift 4. For now, we have queries, and bare objects. The UserType is also exposed, need some work for archival / surviving the app restarts etc... This could be ported from the Obj-C SDK, leveraging the Keychain as it is for storing sensitive data. Installation is the next big one, with the same features as the Obj-C counterpart as well.
What I had in mind:
Also, I'm/we're very open adding collaborators to this project, to get it off the ground as fast as possible! |
Hey Florent! Thanks for the reply. I'd love to be a collaborator on this project. I think it would be a really cool project to work on. So, I've been looking at the code and I agree with a lot of the things that you think should be changed. I think that if we plan on making any major changes to core functionality such as REST API stuff, it should be done early on. That being said, I don't like the current approach. I think it's a little confusing to have the let request = API.Endpoint.signUp.makeRequest(body: data)
request.execute {(response, error) in
} I do actually like the On the other hand, I'm not a huge fan of the way encoding is done right now. I think it's a complex approach that could be simplified greatly by creating a protocol called Finally, should we consider renaming Let me know what you think about these things! As I said, I think we should tackle these issues before we move onto implementing more functionality because it's much easier to change these things early on. |
Yeah and
This would basically be:
not sure it's worth formalizing into an extension, note that some keys have to be skipped, and if you use a default JSON encoder this won't work. This is the whole point of Codable, and it provides Compile time encoder instead of runtime encoder which is way better. the encoder on the other hand, is the ParseEncoder, dumped / modified from the date. I would not change that at the moment, it simplifies all aspects of object mirroring to discover the keys to encode etc....
I'm not against, I was concerned it would feel weird at consumption time:
Hope that helps! |
Yes, that's great! You're OK with my proposed changes to |
yep I had the feeling for a while RESTCommand and API should be merged (the more I started adding enum based endpoints to API) everything started to be redundant. Also perhaps keep in mind that we may want LocalDataStore, in that case, having a front on top of API isn't BAD, but we may need 2 API abstractions or I dunno. |
@flovilmart Can you take a look at the commit I just added to my fork of the repository? API.Endpoint.someEndpoint.makeRequest {(data: Data?, error: Error?) in
if let data = data {
do {
let decoded = try getDecoder().decode(Something, from: data)
callback(decoded, nil)
} catch {
callback(nil, error)
}
else if let error = error {
callback(nil, error)
} else {
fatalError()
}
} It's not totally gross, but I do think we could do something to clean that up a bit. Do you think that's necessary? One thing that bugs me the most is the |
Unknown / internal error would be nice, actually we know what happens, neither data nor error was set. |
Got it. |
Also fatalError could help us in unit tests to make sure we cover all bases |
Which one should we have then? |
Internally, we should never reach that state, so that’s a good candidate for fatalError. We can guarantee as SDK developers that it is never reached!
…On Sep 10, 2017, 19:02 -0400, Pranjal Satija ***@***.***>, wrote:
Which one should we have then?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@flovilmart, do you have any advice on how to implement |
Have a look at the original SDK, that will help a lot. It involves disk serialization and keychain for sensitive info like sessionTokens |
@flovilmart I have a quick question, and it may just be caused by me not understanding something. I found this in PFCurrentUserController *controller = self.coreDataSource.currentUserController;
return [[controller saveCurrentObjectAsync:user] continueWithBlock:^id(BFTask *task) {
return user;
}]; I then headed to - (BFTask *)_saveSensitiveUserDataAsync:(PFUser *)user toKeychainItemWithName:(NSString *)itemName {
return [BFTask taskFromExecutor:[BFExecutor defaultPriorityBackgroundExecutor] withBlock:^id{
NSMutableDictionary *userData = [NSMutableDictionary dictionaryWithCapacity:2];
@synchronized (user.lock) {
if (user.sessionToken) {
userData[PFUserSessionTokenRESTKey] = [user.sessionToken copy];
}
if (user.authData.count) {
userData[PFUserAuthDataRESTKey] = [user.authData copy];
}
}
self.commonDataSource.keychainStore[itemName] = userData;
return nil;
}];
} Finally, I went to The only thing I'm confused about in all this is that it seems like the Additionally, do I have the right idea here? Is this how users are actually persisted in the Objective C version of the SDK? I'm not familiar with it, as I never made any code level changes to it, but I think I'm seeing how the user object is persisted. Could you correct me if I'm wrong? |
You are 💯 right about the objective C SDK. For now we could have a file based local data store as it’s not provided yet by the SDK! |
Alright. I'll use one of the REST encoders and have the SDK save the current user as a JSON file somewhere. For keychain access, should I use something like this, or should I make one myself? I don't think there's much of a reason to make a custom one, but I thought I'd run it by you regardless. |
Yeah, I’d rather keep it as dependency free as possible. The original one is pretty explicit, we could use that as a reference for the base implemenentation? |
I was just going to mention that. I need to step out for a bit but I'll make a PR with my changes with |
you don't need to use any of those, Codable gives us encoding for free so we can encode whatever we want from the object. we can have |
@flovilmart I created a PR and I'll get to work on the persistence stuff tomorrow. |
@flovilmart I'm having a little issue. I'm just finishing up the persistence for
I set a breakpoint, and I found that it fails in func set(object: Any?, forKey key: String) -> Bool {
guard let object = object else {
return removeObject(forKey: key)
}
let data = NSKeyedArchiver.archivedData(withRootObject: object) //this line
var query = keychainQuery(forKey: key)
let update = [
kSecValueData as String: data
]
let status = synchronizationQueue.sync(flags: .barrier) { () -> OSStatus in
if self.data(forKey: key) != nil {
return SecItemUpdate(query as CFDictionary, update as CFDictionary)
} else {
query = query.merging(update) { (_, otherValue) -> Any in otherValue }
return SecItemAdd(query as CFDictionary, nil)
}
}
return status == errSecSuccess
} And I then looked at the debug info for
But I'm stumped as to why this can't be encoded by |
That's very odd indeed. |
There isn't any further information. But the API call is definitely working cause the user was created on the server. |
Nice work guys, how is it going? |
I haven't had a chance to move forward with this project. |
Oh, okay. I'll just stick with the Objective-C library then. Maybe sometime in the future, you'll get back to this I hope. 👍 |
I’m not sure i’ll Have time to work on this project on the forseable future. However if you wish to use this project, you can submit pull requests and enhance it. |
Okay, that'd be nice. I'll see what i can do. |
@flovilmart I see you started https://github.com/parse-community/Parse-Swift/projects/1 Is there anything else that could be added? |
@pranjalsatija I completely understand. I think with your help along with the rest of the people who mentioned they are interested in the project, we should be able to continue where Flo left off. I tested the playground setup and the basic Object, User, user sign-up etc. still works with the latest parse-server. From the dissuasions you linked it seemed like the next steps are to persist the session token to the keychain (this and it's test cases are par with the obj-c one) and work with Installation (I guess technically we can store the User there for now if needed since there's no local storage. I've used the objective-c framework heavily via Swift (I never did any obj-c with it), but I have never made any updates or fixes to that framework, so I've been sifting through the code. Flo pointed out somethings can be converted and reusable. I've been looking here https://github.com/parse-community/Parse-SDK-iOS-OSX/blob/master/Parse/Parse/Internal/User/CurrentUserController/PFCurrentUserController.m. For what's listed under Projects, I'm thinking the order of things that need to be complete can be:
What do you think? Also, API seems to be intact as well and I know that was something you were discussing heavily. Thanks for the review! |
It's great to hear that everything still works! A lot of functionality from the old framework can be mimicked. In the case of I would personally reorder the things a little bit:
In addition, we should rename some stuff to bring it in line with modern Swift naming conventions. I mentioned this above, but it fell through the cracks and we never got to it:
Those are all pretty trivial renames, but the sooner, the better. I feel like they'll get harder to pull off as time goes on. I haven't written any code in this repo for a while, so I'll spend some time reading the code and getting reacquainted with it. I'd be happy to take the lead on implementing Local Storage backed by the filesystem. I'll get started on that + the renaming as soon as your PR is merged. @cbaker6 please let me know if you agree / disagree with any of this stuff. I'd love to hear what you think. |
The reason I put local storage last is due to Flo's comment
It seems Flo believed adding protocols for app developers to conform to so they can use their own local store was the way to go. This seems to be the way many of Apple's frameworks behave. For example, CareKit and ResearchKit store data locally and add hooks for backend developers so any cloud storage can work with the frameworks. Skimming the obj-c framework it seems like it used CoreData for local storage. I'm more than comfortable with writing for CoreData, but it does add more responsibility to this framework along with locking the local storage into one DB. Also, writing the Local Storage component using the protocol approach wouldn't yield us much yet as we still wouldn't be able to store/retrieve anything yet, which is why I mentioned storing the
I did see your previous discussion about this. struct GameScore: ParseSwift.ObjectType {
//: Those are required for Object
var objectId: String?
var createdAt: Date?
var updatedAt: Date?
var ACL: ACL?
//: Your own properties
var score: Int
//: a custom initializer
init(score: Int) {
self.score = score
}
} Having a line like,
I agree.
This sounds good to me.
I think this is a good approach, but maybe we should discuss the local storage a little more based on the previous comments? What are your thoughts about being locked in to a specific local datastore? |
A question that may help us on a direction for LocalDataStore is, to carry out everything projected for version 1.0, what actually needs to be stored locally? I see |
I’d vote for the flexible approach to the local storage problem too. We can always provide a default / reference local storage solution at some point. I used the obj-c LocalDataStore fairly extensively in a client project, only to discover later that there were massive performance issues. I ended up having to hack a new solution together using a 3rd party key / value store database to store objects locally. I had to do some ugly things. I suspect this would have been easier if the SDK were designed for it. |
@drdaz that's a good point. I moved away from using Parse's local datastore in my apps in 2016 and usually design my own CoreData local datastore. The protocol approach will definitely open up 3rd party designs that are optimized for local storage and allow developers to choose what they prefer |
@cbaker6 Indeed - no matter how wonderful a default solution we could provide, there's a good chance that it won't play nice in every use-case. I don't see the protocol approach adding much complexity either. I think the main challenge could be creating a sufficient protocol while not actually implementing something that uses it. |
To help mitigate the challenges we can probably have a discussion about what's critical for the protocol to have. In addition, if the localDataStore is designed as a protocol, we can skip the synchronization issues, which are a whole different beast as syncing with a wall clock isn't ideal, especially when multiple devices are tied to an individual account. 3rd parties can then add their own syncing solutions to local storage. I'm assuming there will need to be some type of cache for things like |
@cbaker6 I like the approach of just writing a protocol and then letting other people implement it to provide their own local data store. That's a great idea and I agree that it keeps the core of this framework small; in the future, we can even break different implementations out into different pods so we don't have a bloated binary. I think for now, for what we need to persist for As for the naming, what about renaming Finally, when it comes to designing a local storage protocol, I think there are roughly 2 different use cases we need to support:
We can define 2 protocols then, something like protocol PrimitiveObjectStore {
func delete(valueFor key: String) throws
func get<T: Codable>(valueFor key: String) throws -> T
func set<T: Codable>(value: T, for key: String) throws
} We can have a single implementation available for now that relies on the Keychain, and maybe an in-memory or filesystem one for debugging if we need it. @cbaker6 and @drdaz how does that sound? If there's no opposition, I can get started on this stuff soon. |
Sounds good to me. The separate pods/packages can easily add ParseSwift as a dependency. This would look similar to the adapters for parse-server (the outside ones anyway). I can see this occurring with FacebookUtils, TwitterUtils, etc.
I like it!
These both sound good to me as well! @pranjalsatija I don't know who all has merging ability on this repo, but looks like you and ShawnBaek were the last ones to contribute code and review capabilities. Was there anything setup for who accepts merges? |
Sweet, I'll get started. And I'm not sure who has merge access. I definitely don't, unfortunately. I'm pretty sure this repo is lacking a maintainer right now though, so it shouldn't be too difficult to get access. cc @drdaz @dplewis @TomWFox (those are just people I've seen semi-active around here lately, please let me know if we should talk to someone else) |
Currently the whole @parse-community/ios-osx team has write access, that includes @drdaz @noobs2ninjas @mrmarcsmith & @mtrezza. I do too as part of the core team as do the others although I'm the only one who engages in the iOS repos. I think that should be ok for now, but I'll keep an eye out and you can always ping me if there's an issue. @cbaker6 @pranjalsatija I could get either or both added to this repo at some point if you remain active and that's of interest? |
Thanks @TomWFox! I’m not able to view the iOS-OSX community, it may not be public. I’m fine with being added in the future as I plan to make some more updates here. Are you able to take a look at the PR? We should be able to start making updates after that PR is merged |
@TomWFox Ditto for me! |
Annoyingly, it seems you can't make GitHub teams viewable to non-members. I'll take a look at the PR, although I have no experience with this codebase apart from some minor experimentation. if you guys are happy, I'm happy as this SDK isn't/shouldn't be used in production yet anyway, but it's probably worth waiting a bit to merge incase anyone else wants to chime in now. |
@TomWFox it looks like 2 people (one of them being @pranjalsatija the other @seanbaek) who were coding and conversing with Flo have reviewed the PR and approved. My assumption is from a historical perspective @pranjalsatija and @seanbaek are probably most familiar with Flo's vision and criticism for this framework. Also @drdaz mentioned he had made some similar changes in the past. It has definitely had some eyes on it. I made sure to bring everything up-to-standard along with bug fixes, but didn't add any features on purpose. This PR is enable the rest of the community to develop |
Good to know. I'm not going to hold this up for days, it's just standard practice to allow team members some time to take a look. I won't review now (it's 2am here) but I'll try and check it out in the next 24hrs, in the meantime @drdaz or someone else may well review and merge. |
I'll also add I don't think this too far away from production. Flo demonstrated the basic features of login and creating objects in Swift Playgrounds, which all still work fine. Between the people who are interested in the framework and our knowledge of parse-server and this framework, I'm optimistic about knocking out the roadmap for v1 pretty quickly |
Glad to hear it, I will do my best to support contributors and encourage interest in this SDK. |
@flovilmart I'm really interested in helping to make this a thing, and I'd love to take on some tasks (major ones, too!) to get this up to feature parity with the Objective C version. Are there any evolution guidelines, or a list of long and short term goals and features? If so, could you please point me to them so I can begin to get to try implementing some of the features that are planned? If not, could you just give me some general advice on some of the things that are planned? I'd love to have an active role in the development of this framework.
The text was updated successfully, but these errors were encountered: