forcedotcom / SalesforceMobileSDK-Templates

BSD 3-Clause "New" or "Revised" License
52 stars 56 forks source link

MobileSyncExplorerSwift with dynamic sync targets #263

Closed siilats closed 4 years ago

siilats commented 4 years ago

Hi please find a pull request that makes a sample create a syncUp and syncDown based on the object fields and use MobileSync publisher for both up and down sync. This means you do not need to hard code the column names. I used Barista-IOS as inspiration.

salesforce-cla[bot] commented 4 years ago

Thanks for the contribution! Before we can merge this, we need @siilats to sign the Salesforce.com Contributor License Agreement.

siilats commented 4 years ago
  1. I made class names more Swift like
  2. Used the "single source of truth" to remove the ContactInput class
  3. used the codable in dataspec
siilats commented 4 years ago

I used SalesForce cloud services project Barista IOS as a guide. There you have the store done nicely.

On Tue, Apr 28, 2020 at 12:15 PM Raj Rao notifications@github.com wrote:

@trooper2013 commented on this pull request.

@siilats https://github.com/siilats Let me start by saying that we appreciate all the work you have put in here. There are benefits for some of the ideas proposed in this PR. But I'm afraid that we may have set a precedent by using SObjectDataManager and SObject in our template. Our big goal for templates is to make it easy for developers to understand sdk concepts. We are generally gravitating towards simpler and lesser code. So in particular I would like to highlight the complexity around the use of StoreProtocol. We would prefer a simplified model, where the Contacts app just deals with Contact storage and sync. I also have some general concerns about the protocol's design. I will refrain from doing a detailed review for now. Generally speaking if this were a path that we would agree on, the we would have preferred a simpler composition for the StoreProtocol.swift and its dependencies. Perhaps the StoreProtocol and extensions for the sync/store could hold value just by itself as an extension? Thoughts?

In MobileSyncExplorerSwift/MobileSyncExplorerSwift/Helpers/SyncManager.swift https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/263#discussion_r415949509 :

@@ -0,0 +1,82 @@

+//

+// SyncManager.swift

+// MobileSyncExplorerSwift

+//

+// Created by keith siilats on 4/19/20.

+// Copyright © 2020 MobileSyncExplorerSwiftOrganizationName. All rights reserved.

+//

+

+import Foundation

+import SmartStore

+import MobileSync

+import Combine

+

+extension SyncManager {

What is the reason not to use the stock SyncManager.reSyncWithoutUpdates and SyncManager publishers in the mobile app? The SDK has a basic extension that does what this extension is accomplishing.

In MobileSyncExplorerSwift/MobileSyncExplorerSwift/SObjects/Store.swift https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/263#discussion_r416695734 :

+//

+

+import Foundation

+import SmartStore

+import MobileSync

+import Combine

+

+class Store: ObservableObject {

  • @Published var items: [objectType] = []

  • private var cancellableSet: Set = []

  • private let kMaxQueryPageSize: UInt = 1000

  • var syncMgr: SyncManager

  • var dataSpec: SObjectDataSpec

  • public let sqlQueryString: String = RestClient.soqlQuery(withFields: objectType.createFields, sObject: objectType.objectName, whereClause: nil, groupBy: nil, having: nil, orderBy: [objectType.orderPath], limit: 100)!

We should not force unwrap the result here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/263#pullrequestreview-401107445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAI2ZY3QULYP3ER4QN5SOVLRO36ITANCNFSM4MMDNSKA .

-- Keith Siilats keith@siilats.com www.bytelogics.com Fax: 646-352-4705 Phone: +1 (646) 881-4378

siilats commented 4 years ago

https://github.com/SalesforceCloudServices/barista-iOS

On Tue, Apr 28, 2020 at 12:15 PM Raj Rao notifications@github.com wrote:

@trooper2013 commented on this pull request.

@siilats https://github.com/siilats Let me start by saying that we appreciate all the work you have put in here. There are benefits for some of the ideas proposed in this PR. But I'm afraid that we may have set a precedent by using SObjectDataManager and SObject in our template. Our big goal for templates is to make it easy for developers to understand sdk concepts. We are generally gravitating towards simpler and lesser code. So in particular I would like to highlight the complexity around the use of StoreProtocol. We would prefer a simplified model, where the Contacts app just deals with Contact storage and sync. I also have some general concerns about the protocol's design. I will refrain from doing a detailed review for now. Generally speaking if this were a path that we would agree on, the we would have preferred a simpler composition for the StoreProtocol.swift and its dependencies. Perhaps the StoreProtocol and extensions for the sync/store could hold value just by itself as an extension? Thoughts?

In MobileSyncExplorerSwift/MobileSyncExplorerSwift/Helpers/SyncManager.swift https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/263#discussion_r415949509 :

@@ -0,0 +1,82 @@

+//

+// SyncManager.swift

+// MobileSyncExplorerSwift

+//

+// Created by keith siilats on 4/19/20.

+// Copyright © 2020 MobileSyncExplorerSwiftOrganizationName. All rights reserved.

+//

+

+import Foundation

+import SmartStore

+import MobileSync

+import Combine

+

+extension SyncManager {

What is the reason not to use the stock SyncManager.reSyncWithoutUpdates and SyncManager publishers in the mobile app? The SDK has a basic extension that does what this extension is accomplishing.

In MobileSyncExplorerSwift/MobileSyncExplorerSwift/SObjects/Store.swift https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/263#discussion_r416695734 :

+//

+

+import Foundation

+import SmartStore

+import MobileSync

+import Combine

+

+class Store: ObservableObject {

  • @Published var items: [objectType] = []

  • private var cancellableSet: Set = []

  • private let kMaxQueryPageSize: UInt = 1000

  • var syncMgr: SyncManager

  • var dataSpec: SObjectDataSpec

  • public let sqlQueryString: String = RestClient.soqlQuery(withFields: objectType.createFields, sObject: objectType.objectName, whereClause: nil, groupBy: nil, having: nil, orderBy: [objectType.orderPath], limit: 100)!

We should not force unwrap the result here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/forcedotcom/SalesforceMobileSDK-Templates/pull/263#pullrequestreview-401107445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAI2ZY3QULYP3ER4QN5SOVLRO36ITANCNFSM4MMDNSKA .

-- Keith Siilats keith@siilats.com www.bytelogics.com Fax: 646-352-4705 Phone: +1 (646) 881-4378

trooper2013 commented 4 years ago

@siilats I would like to highlight a sentiment in my PR comments that some of the extensions you have added are best extracted and hosted in your own repo. This PR adds more complexity to the current app, and as such the team's long term goals are to simplify the template and showcase our api(s) within the template app. For that reason, I will close the PR.