snowplow / snowplow-javascript-tracker

Snowplow event tracker for client-side and server-side JavaScript. Add analytics to your websites, web apps and servers.
http://snowplowanalytics.com
BSD 3-Clause "New" or "Revised" License
555 stars 222 forks source link

BrowserTracker: fix method type definitions #1284

Closed jethron closed 10 months ago

jethron commented 10 months ago

Per the TypeScript docs, functions that return values are type-compatible with functions declared as having void return type; however if you're consuming these type definitions this becomes annoying as you are required to cast as unknown as string at the call sites to get the correct types actually returned from these methods. Because the types are compatible tsc never reported any issue with these methods.

bundlemon[bot] commented 10 months ago

BundleMon

Files added (6) Status | Path | Size | Limits :------------: | ------------ | :------------: | :------------: :white_check_mark: | libraries/browser-tracker-core/dist/index.mod
ule.js
| +26.51KB | 27KB / +10% :white_check_mark: | trackers/javascript-tracker/dist/sp.js
| +25.1KB | 25.5KB / +10% :white_check_mark: | trackers/javascript-tracker/dist/sp.lite.js
| +15.4KB | 16KB / +10% :white_check_mark: | trackers/browser-tracker/dist/index.umd.min.j
s
| +15.29KB | 15.5KB / +10% :white_check_mark: | libraries/tracker-core/dist/index.module.js
| +13.36KB | 15KB / +10% :white_check_mark: | trackers/browser-tracker/dist/index.module.js
| +3.51KB | 5KB / +10%

Total files change +99.17KB 0%

Final result: :white_check_mark:

View report in BundleMon website ➡️


Current branch size history

jethron commented 10 months ago

My concern is whether this should wait for the v4 release or can go out in v3? I think this might be a breaking change and can produce type errors for users who previously worked around the void return type by type casting the return values. What do you think?

I'm not sure this would really happen: void isn't castable to any other type AFAIK, so users would have likely had to use either: as unknown as T, as any, or as undefined (though I don't think this would be useful, so unlikely); casting straight from void to anything more specific would have caused errors by itself. So any cast, even if it was incorrect, would have been erased by unknown/any and will avoid an error even if that cast isn't compatible with these changes.

So I think it's safe for V3, but can be persuaded otherwise if you have an example where this is problematic?