Skip to content

Improve Analytics.track(name:properties:) to get compile time assurances that properties will encode #197

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

Closed
rtharston opened this issue Mar 6, 2023 · 4 comments · Fixed by #202
Assignees
Labels

Comments

@rtharston
Copy link

rtharston commented Mar 6, 2023

Is your feature request related to a problem? Please describe.

At first glance I thought Analytics.track(name:properties:) could take any Codable type in the properties dictionary since everything is eventually encoded to JSON, but after closer inspection I discovered that isn't the case sine you use a custom JSON enum that only handles a subset of what can be encoded to JSON.

Here is a place in our code where that assumption is visible:

// If the value is Codable then leave it as is and let Segment encode it into JSON, otherwise make use of CustomStringConvertible
let loggingValue: Any = value as? Codable ?? value.description
analytics.track(name: segmentEvent, properties: ["value": loggingValue])

Here I am checking if the value we want to log is Codable and if it is that gets sent as is, and if it isn't then get a string description to send instead. That way the value can be sent with as much information as possible when it is Codable.

Problem is this leads to a crash in DEBUG builds when value is Codable but isn't handled by the JSON enum (like Set).

Describe the solution you'd like

I don't know the history of why a JSON enum is used instead of Codable (or just Encodable) and JSONEncoder, but if it is possible to update Analytics.track(name:properties:) to use [String: Encodable] instead of [String: Any] and then use JSONEncoder to encode the dictionary I think that would be the simplest, and would provide the strongest compile time guarantees.

A change like that would be disruptive enough to warrant waiting 2.0, but I'd love to see it earlier, so perhaps the old can be kept and marked deprecated in 1.4 and then removed in 2.0.

Describe alternatives you've considered

If you can't use the standard JSONEncoder for some reason, then a custom protocol can be used instead of Encodable in place of Any and extend the supported types in JSON with that protocol. Something like SegmentSerializable.

public func track(name: String, properties: [String: SegmentSerializable]? = nil)

In JSON.swift:

public extension String: SegmentSerializable {}
public extension Array: SegmentSerializable where Element: SegmentSerializable {}
etc...

Additional context

The Encodable solution would be fit in with the Swift ecosystem, as it would allow us to send anything we want to Segment by making it Encodable, and give us compile time assurance that what we are sending will make it to Segment.

If that is deemed impossible for reasons I can't see from the outside I will accept the alternative where at the very least we get compile time assurances.

@bsneed
Copy link
Contributor

bsneed commented Mar 7, 2023

Thanks @rtharston! I'll dive in and see what I can do. We use the JSON type as an intermediary because there's a lot to an event payload that is freeform and doesn't fit nicely into a Codable type.

@rtharston
Copy link
Author

Thanks for looking at this @bsneed.

I'm curious what you mean by 'freeform', and what you are adding to the payload that isn't Codable. Everything that is supported in the JSON enum in JSON.swift is a Codable type. (Arrays and Dictionaries are Codable if their contents are Codable.)

Of course I understand that asking to replace JSON with Codable is a big task and there are likely gotchas I don't know about because I haven't worked on this codebase, but from my limited, outside view it seems like it should be possible. 😁

@bsneed
Copy link
Contributor

bsneed commented Mar 10, 2023

@rtharston once we dive into plugins and whatnot, we don't really have any control of how or what people will modify, add or remove. Unstructured dictionaries break the codable paradigm and there's a lot of unstructured bits. I am having a look how I can tighten this up though.

@bsneed
Copy link
Contributor

bsneed commented Mar 10, 2023

@rtharston there's also a track<T: Codable>(... properties: T?) that might be useful to you, not sure. Lemme know what you think of this change. Seems to be just what you asked for at the call site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants