feat: Do not return null as analyticsUrl for CI=true

[pgrzesik]

We would like to record events from CI as well as we plan to introduce CI detection and enrich analytics events with that information

Note: The plan is to release it as a major version to avoid changing behavior for older versions which won't include information about CI

[codecov]

Codecov Report

Merging #76 (3350702) into master (41cd2c1) will increase coverage by 1.28%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   90.14%   91.42%   +1.28%     
==========================================
  Files           9        9              
  Lines         213      210       -3     
==========================================
  Hits          192      192              
+ Misses         21       18       -3     

Impacted Files	Coverage Δ
analytics-and-notfications-url.js	0.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41cd2c1...b5318af. Read the comment docs.

[medikoo]

Thanks @pgrzesik

If we want to publish it as breaking change (which triggers new major release), we need to indicate this as breaking change in commit message.

[pgrzesik]

Totally 👍 I've also started wondering a bit more about this specific check in wider context and what impact it could have e.g. on components - if I understand correctly, at the moment components only use that to trigger notifications and it totally makes sense to avoid triggering notifications in CI, not only for components but for Framework as well. Do you think it's safe @medikoo to change it as if I'm not mistaken it will report notifications for CI for people now?

[medikoo]

if I understand correctly, at the moment components only use that to trigger notifications and it totally makes sense to avoid triggering notifications in CI

I'm not sure if it's a big concern. I think main reason for making this a breaking change, is that thanks to that we will not mix untracked and tracked CI events in scope of same version of a Framework, and that will allow us to keep data reliable in metrics service Dynamodb

[pgrzesik]

That's correct @medikoo - what I was worried about is that now additonally, for all new versions, notifications can be also triggered for CI deployments, which this setting was preventing in a lot of cases.

[medikoo]

👍

I would just reword the commit. At least No longer return null feels very internal, and implementation specific.

I think it should be more something as: Expose analytics server URL unconditionally (and not just in non-CI environments)..

This message is expected to land as is in changelog, and unless justified I would not describe changes using JS language specific terms. Ideally if implementation details are black boxed

[pgrzesik]

Good suggestion, I've reworded the commit a little bit 👍

[medikoo]

Looks great 👍