Skip to content

Conversation

@eligutovsky
Copy link
Member

@eligutovsky eligutovsky commented Dec 14, 2023

Description of Changes

Use GenericJSON that conforms to Codable

Breaking Changes

  • None

Release Checklist

Prepare:

  • Detail any breaking changes. Breaking changes require a new major version number
  • Check pod lib lint passes
  • Update any relevant sections of the repository wiki pages on a branch

Bump versions in:

  • OptimoveCore.podspec

  • OptimoveNotificationServiceExtension.podspec

  • OptimoveSDK.podspec

  • OptimoveCore/Sources/Classes/Constants/SDKVersion.swift

  • README.md

  • CHANGELOG.md

  • Update major version numbers in wiki (basic integration + push guides)

Integration tests

T&T Only

  • Init SDK with only T&T credentials
  • Associate customer
  • Associate email
  • Track events

Mobile Only

  • Init SDK with all credentials
  • Track events
  • Associate customer (verify both backends)
  • Register for push
  • Opt-in for In-App
  • Send test push
  • Send test In-App
  • Receive / trigger deep link handler (In-App/Push)
  • Receive / trigger the content extension, render image and action buttons for push
  • Verify push opened handler

Deferred Deep Links

  • With app installed, trigger deep link handler
  • With app uninstalled, follow deep link, install test bundle, verify deep link read from Clipboard, trigger deep link handler

Combined

  • Track event for T&T, verify push received
  • Trigger scheduled campaign, verify push received
  • Trigger scheduled campaign, verify In-App received

Release:

  • Squash and merge to master
  • Delete branch once merged
  • Create tag from master matching chosen version
  • Run pod trunk push to publish to CocoaPods

Post Release:

  • Push wiki pages to master

@eligutovsky eligutovsky changed the title refactor: json refactor: json [7] Dec 14, 2023
],
dependencies: [
.package(url: "https://github.com/WeTransfer/Mocker", from: "3.0.1"),
.package(url: "https://github.com/iwill/generic-json-swift", from: "2.0.2"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @eligutovsky , have we had any discussion about adding new dependencies to the SDK?

Some clients are a little sensitive to the size/dependency footprint, and dependencies can make integration more tricky if there are conflicting versions.

Just a thought, we should consider it carefully.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency was copy-pasted to the source code previously. I will remove this dependency and will keep it copy-pasted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry not sure I understand, you mean we already depend on it? (if so, fair enough, might propose the inverse of dependency simplification in the future, but that's another topic 😅)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we're already depend on it. See OptimoveSDK/Sources/Classes/JSON/JSON.swift -- in removed files of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, clear!

Next question: do you know why we 'vendored' this library code into the SDK in the past, does it matter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added previously because the SDK allows to get [String: Any] as a parameter of an event.

See #97 (comment)

It might worth to replace JSON with Data by using JSONSerialization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we need to decode any internal json to a Codable model.

private(set) var dismissedAt: Date?
private(set) var sentAt: Date
private(set) var data: NSDictionary?
private(set) var data: ObjcJSON?
Copy link
Contributor

@cgwyllie cgwyllie Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these 'breaking' API changes / do we expose/leak the type from a library to the consumer app?

Copy link
Member Author

@eligutovsky eligutovsky Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CoreData entity model is only meant for internal use and should not be accessed by consumer apps.
This change has been made to fix the issue of NSDictionary not conforming to the Codable protocol. It is important to ensure that values stored in the CoreData are Codable before they are stored. This will help ensure that the data is ready to be send through network.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, all for these kind of things in general, Codable will be simpler, but I think this model InAppInboxItem is not the Core Data entity, it is the model we expose to consumers, so changing this field could be breaking/leaky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this class cannot be exposed to a consumer -- it is not public

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants