Skip to content
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

REPLAY-1936 Session Replay Objective-C interface #1417

Closed
wants to merge 5 commits into from

Conversation

maxep
Copy link
Member

@maxep maxep commented Aug 16, 2023

What and why?

Expose Session Replay to Objective-C.
Fix #1387

How?

This PR includes 2 proposals:

1. Additional Interface f607fd5

Pretty much like the DatadogObjc module,DatadogSessionReplay will expose specific objects that support interoperability, see the following example:

@objc(DDSessionReplay) @available(swift, obsoleted: 1)
public final class objc_SessionReplay: NSObject {

    @objc @available(swift, obsoleted: 1)
    public static func enable(with configuration: objc_SessionReplayConfiguration)
}

It's using the @available(swift, obsoleted: 1) annotation to only expose those to objc.

This solution has 2 main drawbacks:

  • Definitions are duplicated.
  • The wrappers are not testable: we cannot access the inner values from swift.

2. Single Interop Interface 6aadf46

By doing so, we remove the duplication in wrappers and fully leverage Swift/Objective-C interoperability.

It's made possible by the following facts:

  • SessionReplay is not instantiable and only expose static method.
  • SessionReplayConfiguration can be mutated freely before enabling the Feature, the instance is never retained.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@maxep maxep self-assigned this Aug 16, 2023
@maxep
Copy link
Member Author

maxep commented Aug 16, 2023

This PR is in draft as it contains 2 solutions described above. I'm in favor of solution 2 since it doesn't have any impact on the configuration and gives us interoperability almost for free.

REPLAY-1936 use interop interface

Update SessionReplayConfiguration.swift
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Aug 16, 2023

Datadog Report

Branch report: maxep/REPLAY-1936/sr-objc
Commit report: d9c3e0b

dd-sdk-ios: 0 Failed, 0 New Flaky, 118 Passed, 0 Skipped, 2m 17.86s Wall Time

@ncreated
Copy link
Member

Additional Interface f607fd5

The fact that we can't test code decorated with:

@objc(DDSessionReplay) @available(swift, obsoleted: 1)

breaks the main promise behind the idea of using it.

Single Interop Interface 6aadf46

By doing so, we remove the duplication in wrappers and fully leverage Swift/Objective-C interoperability.

It's made possible by the following facts:

  • SessionReplay is not instantiable and only expose static method.
  • SessionReplayConfiguration can be mutated freely before enabling the Feature, the instance is never retained.

There is a large risk in this approach hidden in the fact that we blend both interfaces into one. The more powerful Swift interface will be constrained by features only available in Swift <> Objc interoperability. It means we won't be able to use enums with associated values, structs, Encodable attributes, generics and other Swift idioms in public API. The @objc compromise would need to be taken for each next public API we add to SR.

I understand that the simplistic SR interface is not a problem for @objc today, but what makes us think it will be same true as the product evolves? Keep in mind that SR is a new product, not yet GA. For example, the same observations used in 6aadf46 were true in early days of Logs, Trace and RUM APIs, but as we grew, it changed. Today we leverage many Swift capabilities not available in @objc interop: from user attributes [String: Encodable] to lightweight RUM event structs in mapper APIs.


All that said, I believe that defining SR in DatadogObjc the same way we did for other products should be the way to go today. Modularizing DatadogObjc should be another topic that requires RFC with a discussion of requirements, constraints and risks.

@maxep
Copy link
Member Author

maxep commented Aug 17, 2023

Thanks, @ncreated !

There is a large risk in this approach hidden in the fact that we blend both interfaces into one. The more powerful Swift interface will be constrained by features only available in Swift <> Objc interoperability.

I don't consider this a risk but a constraint. Any @objc interface can't use Swift idioms, but it doesn't mean they can't coexist. The following example is a valid approach:

public var attributes: [String: Encodable] = [:]

@objc(attributes)
public var objc_attributes: [String: Any] = [:] {
    didSet { attributes = objc_attributes.mapValues(AnyEncodable.init) }
}

Duplicating interfaces in a separate module is more maintenance cost than complying to swift/objc interoperability from the main api, especially for such simple interface. I'm aware that the product will grow in complexity, but nothing we can't overcome with an interop api. In RUM for example, we would have to use wrappers for some structs and @objc variants like in the example above, but it's totally feasible.


I understand that this proposal differs from the current (intermediary) state of the objc interfaces, and I agree to continue exposing it in DatadogObjc for now. But we should have a plan for modularising/removing the DatadogObjc, I will create a ticket for an RFC to expose various solutions we have for objc interop.

@maxep
Copy link
Member Author

maxep commented Aug 17, 2023

Closing in favor of #1419

@maxep maxep closed this Aug 17, 2023
@ncreated
Copy link
Member

There is a large risk in this approach hidden in the fact that we blend both interfaces into one. The more powerful Swift interface will be constrained by features only available in Swift <> Objc interoperability.

I don't consider this a risk but a constraint. Any @objc interface can't use Swift idioms, but it doesn't mean they can't coexist. The following example is a valid approach:

public var attributes: [String: Encodable] = [:]

@objc(attributes)
public var objc_attributes: [String: Any] = [:] {
    didSet { attributes = objc_attributes.mapValues(AnyEncodable.init) }
}

@maxep Indeed, that seems to be fair proposal and I like it 👍. However, there are still things to define on top of it, e.g.: should we go "interop if possible" or "Swift first, interop as extension". Both could shape the codebase differently depending on implementation guidelines (e.g. dictate no use of structs in public APIs; invite for mixing declarations in a single file or ask to isolate them; single test cases vs distinct ones). All this could be a good discussion for an RFC 👌.

(...) But we should have a plan for modularising/removing the DatadogObjc, I will create a ticket for an RFC to expose various solutions we have for objc interop.

💯 🎯

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.

Session Replay Beta - in objective - c
2 participants