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

Support exactOptionalPropertyTypes in TypeScript #1138

Closed
mrmckeb opened this issue Sep 4, 2024 · 4 comments · Fixed by #1156
Closed

Support exactOptionalPropertyTypes in TypeScript #1138

mrmckeb opened this issue Sep 4, 2024 · 4 comments · Fixed by #1156

Comments

@mrmckeb
Copy link
Contributor

mrmckeb commented Sep 4, 2024

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
Copy link
Contributor

silesky commented Sep 4, 2024

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

@mrmckeb
Copy link
Contributor Author

mrmckeb commented Sep 7, 2024

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
Copy link
Contributor

silesky commented Sep 10, 2024

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

@mrmckeb
Copy link
Contributor Author

mrmckeb commented Sep 23, 2024

Done - sorry it took so long @silesky!
#1156

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

Successfully merging a pull request may close this issue.

2 participants