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

[BUG] ITelemetryItem uses intersection type instead of union type for tags property #2258

Closed
devm33 opened this issue Feb 4, 2024 · 4 comments
Assignees
Milestone

Comments

@devm33
Copy link

devm33 commented Feb 4, 2024

The ITelemetryItem interface uses an intersection type (&) between Tags and Tags[] instead of a union type (|). There's a note on the line that Tags[] will be deprecated:

https://github.com/microsoft/ApplicationInsights-JS/blame/f9d5ec1e2045f6206c8d7dc9b0cb3e6509f45473/shared/AppInsightsCore/src/JavaScriptSDK.Interfaces/ITelemetryItem.ts#L37

However using an intersection type forces passing an array in order to satisfy the constraints of Tags & Tags[]:

Type '{ [key: string]: string; }' is not assignable to type 'Tags & Tags[]'.
  Type '{ [key: string]: string; }' is missing the following properties from type 'Tags[]': length, pop, push, concat, and 29 more.
@MSNev MSNev modified the milestones: 3.x.x (Future Release), 3.1.x Feb 5, 2024
@MSNev
Copy link
Collaborator

MSNev commented Feb 5, 2024

Just had a quick look at the code and I think we can just remove the Tags[] as Tags is defined (and used -- based on quick look) as an object with dynamic keys -- so the only "real" usage would be if this is set as a tag with a numeric key.

Actually, we have 2 tags objects and the ITelemetryItem one is initialized as an array.

@siyuniu-ms can you have a look and see the impact of removing the Array definition, I suspect that we might need to "grow" some ArrayLike support for the Tags interface without updating all of our tests.

While I initially thought that this would be a good candidate for 3.1.0, I'm not so sure now.

@devm33 Which version of TypeScript is throwing this error?

@MSNev
Copy link
Collaborator

MSNev commented Feb 5, 2024

Also, because of backward compatibility support, this might be problematic -- so as suggested it might just be easier to change the definition to a union type as suggested (rather than removing the Tags[] definition completely.

@devm33
Copy link
Author

devm33 commented Feb 5, 2024

@MSNev I'm running 5.1.3. The point I was trying to raise was the current typing is preventing passing Tags. It only accepts an array. Which based on the comment seemed like the opposite of the intention.

@MSNev
Copy link
Collaborator

MSNev commented Feb 5, 2024

Yeah, it looks like someone in the past tried to convert this to an object (and when we construct the outbound serialized form this is what we do), and while this definition "works" (from a previous version and raw JS perspective), it's problematic -- especially if the latest versions of TS are now enforcing this more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants