segmentio / analytics-next

Segment Analytics.js 2.0
https://segment.com/docs/connections/sources/catalog/libraries/website/javascript
MIT License
410 stars 135 forks source link

Support `exactOptionalPropertyTypes` in TypeScript #1138

Closed mrmckeb closed 1 month ago

mrmckeb commented 2 months ago

This is a quick change, and I'm happy to contribute.

When using TypeScript with exactOptionalPropertyTypes, we get errors because Segment types don't allow undefined to be supplied to optional values.

The fix is that whenever a type is optional, it should also allow undefined.

As an example:

export type TrackParams = {
  event: string
-  properties?: EventProperties
+  properties?: EventProperties | undefined
-  context?: ExtraContext
+  context?: ExtraContext | undefined
-  timestamp?: Timestamp
+  timestamp?: Timestamp | undefined
-  integrations?: Integrations
+  integrations?: Integrations | undefined
-  messageId?: string
+  messageId?: string | undefined
} & IdentityOptions
silesky commented 2 months ago

@mrmckeb makes sense, happy to accept a PR here. Thanks!

mrmckeb commented 2 months ago

I took a look at this and I see two options:

  1. Codemod the whole codebase, so that every "optional" value accepts undefined - I have that mostly done, but it's a big PR.
  2. Change only what we need, and over time, other people can create PRs if they find additional areas where they'd like to have undefined supported.

Either option will likely require more changes over time, as option 1 still won't prevent new code from being added that doesn't include undefined when the type is optional.

What do you think @silesky?

silesky commented 1 month ago

@mrmckeb -- definitely the one with the least amount of code changes, that ideally only touches the public APIs (i.e. Params). Thanks!

mrmckeb commented 1 month ago

Done - sorry it took so long @silesky! https://github.com/segmentio/analytics-next/pull/1156