sharedAnalytics returns null before setup

[louoso]

What does this PR do?

changes + (instancetype)sharedAnalytics; to + (instancetype _Nullable)sharedAnalytics;

Where should the reviewer start?

at + (instancetype)sharedAnalytics;

How should this be manually tested?

It shouldn't change any functionality so whatever sanity tests are normally run should suffice.

Any background context you want to provide?

I discovered an instance where an event was attempting to be logged before the client was setup that failed silently. I think this would help prevent that from happening to others.

What are the relevant tickets?

#743

Screenshots or screencasts (if UI/UX change)

Questions:

• Does the docs need an update?

No

• Are there any security concerns?

No

• Do we need to update engineering / success?

¯_(ツ)_/¯

@segmentio/gateway

[codecov-io]

Codecov Report

Merging #744 into master will increase coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   87.61%   87.73%   +0.11%     
==========================================
  Files          36       36              
  Lines        2552     2552              
  Branches      178      175       -3     
==========================================
+ Hits         2236     2239       +3     
+ Misses        311      307       -4     
- Partials        5        6       +1

[f2prateek]

Thanks!

[f2prateek]

So I guess one gripe with this is that it might get annoying to use SEGAnalytics sharedAnalytics as now it will always prompt you for a check, even when you know you've already initialized this (as we recommend doing this in didFinishLaunching)

[louoso]

In our case we did setup Segment in didFinishLaunching, but at some point other code that used segment was inadvertently moved above that setup call. It wasn't obvious that was the case, so we didn't notice until that specific event stopped being logged.

So the tradeoff is how often that happens versus having to change sharedAnalytics to sharedAnalytics? to maintain current behavior.

[gustavosaume]

isn't that why there's an Assert there? I feel there's a clear statement by the library to be initialized before getting the instance. if somehow it's getting called before the developer should get the crash and reorder the calls.

Not only it's annoying to have to unwrap the library every time you want to track something but if a call happens to be executed before it's gonna get ignored making it harder to figure out why.