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

Renaming the module to Analytics was a HUGE oversight #938

Closed
mlfarrell opened this issue Sep 1, 2020 · 22 comments
Closed

Renaming the module to Analytics was a HUGE oversight #938

mlfarrell opened this issue Sep 1, 2020 · 22 comments

Comments

@mlfarrell
Copy link

This is a 3rd party module, it should not use a generic name like Analytics. The rename has completely sandbagged our project and costed me several hours and counting of wasted time trying to find out how to resolve a namespace conflict with our own internal Analytics class. Something related to the NS_SWIFT_NAME() macro inducing a forced rename from SEGAnalytics to the Analytics type causes the SEGAnalytics class to become completely inaccessible inside our codebase due to our own class having the same type name. Every swift module import syntax & typealias trick in the book does not get around the problem..

I get that this change was likely not intended to cause this problem, but it was a huge huge mistake. We may have to actually stop using segment over this...

@migs647
Copy link
Contributor

migs647 commented Sep 1, 2020

Thanks for bringing this to our attention @mlfarrell. We will take a look. 4.0.x is still beta, so we still have the freedom to change this. We will let you know.

@zmian
Copy link

zmian commented Sep 2, 2020

@migs647 Thanks for the reply. I am seeing the same issue as @mlfarrell. This is a huge breaking change and preventing us from upgrading Segment lib to latest version and along side all of the plugins we use (e.g., Amplitude and AppBoy). Module name and class name both referencing Analytics makes it impossible to do any sort of workaround for typealias.

After looking at 902, pretty much all of the classes rename would clash as the module name is too generic (import Analytics) and classes names without SEG prefix is also very generic (e.g., Payload). And any attempt to disambiguate the naming wouldn't work. Consider the following example:

enum Analytics {
    struct Payload { ... }
    ...
    static func track() { ... }
}

If we want to reference Segment's Analytics.Payload vs the project's own instance would require use to namespace the project module instead of Segment (Analytics.Payload)

Would it be possible to name the module Segment? This solves these naming issues. As in the project we can disambiguate by introducing a typealias easily:

// Current
typealias SegmentAnalytics = Analytics.Analytics // ??? this causes all sorts of issues.
Analytics.Payload
...

// Proposed
typealias SegmentAnalytics = Segment.Analytics
Segment.Payload
...

@mlfarrell
Copy link
Author

I got around it by using an ObjC file + briding header to re-expose the analytics instance. It's a workaround for now.

@danielnordh
Copy link

+1 For this causing huge headache. Can no longer build our app and had moved to 4.0 in production due to it not using the advertiser identifier (yes I know it was officially in beta..).

@mlfarrell
Copy link
Author

This is the first I'm hearing about 4.0 being a beta. By removing the version after the right of the podfile line I assumed it would go to the latest STABLE version. That's definitely not a good thing to have users unknowingly update to a beta, guys. Come on.

@bsneed
Copy link
Contributor

bsneed commented Sep 11, 2020

@mlfarrell SPM doesn't support all the extra version identifiers within Xcode currently (x.x.x only), so it forced our hand a bit. We weren't happy about it either, but users have been begging for SPM support for quite some time.

@mlfarrell @danielnordh @zmian
Regarding the original issue about the Swift naming, I'm currently working on this. I'll share my thoughts with you guys ...

Approach #1: We change the module name to be Segment. Seems like a no-brainer solution. However, I've been unable to JUST change the module name. It's looking more and more like I'd need to change the PRODUCT_NAME value to actually make this work. That would then affect ObjC users, React-Native, all the device-mode destinations, etc and would become quite a bit of work. I do think it's the ideal way to go though, but I don't think we'd be able to get it all done for some period of time.

Approach #2: Prefix all Swift names with Segment. This kind of stinks IMO, but it doesn't have the ripple effect #1 has.

Approach #3: Mega-hack and pretty gross, but would solve the problem. Remove all NS_SWIFT_NAME calls, export a Segment enum with a bunch of type aliases to SEGAnalytics, SEGPayload, etc. Gets you Segment.Analytics etc.

What are your thoughts?

@mlfarrell
Copy link
Author

@bsneed. I understand. If it was me, I'd likely keep the repo off the pod spec and have users manually opt-in to the beta via repo checkout or binary framework zip archives.

My vote is for #1. ObjC users have a full arsenal of swizzling, #define-tricks and other stuff to adapt to any changes.

@bsneed
Copy link
Contributor

bsneed commented Sep 11, 2020

@mlfarrell re the versioning, cocoapods would only allow the latest if you had nothing specified. Most have ~> 3.x.

I'll wait for others to respond, but #1 is probably weeks or 1-2 months out given the extent of things that have to change. Not saying no, just an FYI.

@zmian
Copy link

zmian commented Sep 12, 2020

@bsneed Thank you for taking the time to reply to the thread. Option #1 is my vote as well; other options seems to be workarounds. For library such as Segment, IMO it's worth the investment to solve this problem for good with proper module name as Segment.

@bsneed
Copy link
Contributor

bsneed commented Sep 14, 2020

So I'm going to be doing some exploration on this, would any of you be able to test a branch later today?

@mlfarrell
Copy link
Author

We've decided to punt on the segment SDK update during this release cycle due to the confusion surrounding the beta vs not beta update via cocoapods + apple giving a stay on IDFA removal in iOS 14. But I'm down to give it another try in a few weeks.

@zmian
Copy link

zmian commented Sep 15, 2020

@bsneed yes I can try to upgrade to latest version with the changes. Let me know when you are ready. Thanks

@bsneed
Copy link
Contributor

bsneed commented Sep 15, 2020

Thanks @zmian! It's still a work in progress, but a draft is up on the branch "bsneed/swift-rename". If you're using any device-mode destinations, the work there hasn't been completed so there's likely problems. BUT ... the basics I believe are covered. Thanks for providing feedback! ✋

@mlfarrell
Copy link
Author

What's the update on this? We're approaching the warning track regarding submitting apps that use UIWebView but we cannot update until 4.0 is not beta.

@bsneed
Copy link
Contributor

bsneed commented Oct 7, 2020

@mlfarrell our internal date we're targeting is 10/19. However, these changes are now on master. The only hold up on a release are modifying the device mode destinations to work with the name change. Otherwise, what you see on master will be released as 4.1.0 stable.

@rromanchuk
Copy link

Because it wasn't mentioned before, this is also the same swift export name Firebase analytics uses, so it's creating a massive headache/confusion. I don't use segment to replace that device mode sdk yet, so now i've got two obnoxiously named modules 😟

@bsneed
Copy link
Contributor

bsneed commented Oct 23, 2020

Fwiw, the rename is complete. It's now called "Segment" as of 4.1.0.

@bsneed bsneed closed this as completed Oct 23, 2020
@rromanchuk
Copy link

@bsneed https://github.com/segmentio/analytics-ios/blob/4.1.0/Segment/Classes/SEGAnalytics.h#L15

NS_SWIFT_NAME(Analytics)
@interface SEGAnalytics : NSObject

@bsneed
Copy link
Contributor

bsneed commented Oct 24, 2020

@rromanchuk ??

@rromanchuk
Copy link

@bsneed this library exports NS_SWIFT_NAME(Analytics) and NS_SWIFT_NAME(Reachability) and it's really annoying. Why? Because those names are both well established, distributed, recognized modules. When i add this package, my build explodes and i have go in and manually prefix modules for "ambiguous use of" errors, for example import class Reachability.Reachability

And then just general readability and confusion avoidance like below, which in reality is probably less clear because the use of bridging headers.

import FirebaseCore
import Segment

Analytics.shared().track("Bar")
Analytics.logEvent("Foo", parameters: nil)

Why not just play nice and use something reasonable like

NS_SWIFT_NAME(Segment)

NS_SWIFT_NAME(Segment.Reachability)

@bsneed
Copy link
Contributor

bsneed commented Oct 26, 2020

Hi @rromanchuk, Segment's Analytics library existed long before Firebase. None the less, the ticket has been open for quite some time, and we made resolutions and aren't going to make anymore changes around naming. My suggestion would be to setup a type alias, ie:

typealias SegmentAnalytics = Segment.Analytics

or

Segment.Analytics.Shared.track(...)

@vinod93
Copy link

vinod93 commented Apr 6, 2021

In v4.1.3 i'm not able to typealias Segment.Analytics

Its giving me error:
Module 'Segment' has no member named 'Analytics'

Only Segment.AnalyticsConfiguration class is exposed in autocomplete

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

No branches or pull requests

7 participants