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

[main] [Possible Break] Removed Tags[] from ITelemetryItem as this was breaking later versions of TypeScript by using the intersection type instead of union type for tags property #2258 #2269

Merged
merged 10 commits into from
Feb 12, 2024

Conversation

siyuniu-ms
Copy link
Contributor

@siyuniu-ms siyuniu-ms commented Feb 9, 2024

for #2258

function setTageValue(tags: Tags | Tags[], field: string, value: string): void {
// Function body
if (Array.isArray(tags)) {
tags.forEach(tag => setValue(tag, field, value, isString)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look correct... Let me try and repo locally to see the errors.

Copy link
Collaborator

@MSNev MSNev Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looking at the usage a bit deeper and the returned error message, I think we do the following

  • Remove the Tags[] completely so it's just defined as tags?: Tags; as during the serialization process in Serializer.ts we process this field with the _serializeStringMap and if that encounters an "error" it will actually report it as an error (which is what would happen if they do actually pass an incomplete array).
  • Add some release notes about this possible breaking change, at the very least lets call this out in the "name" of this PR, so if someone looks at the history it will be obvious
  • Leave the current "default" initializations as [] as it "seems" happy about that and the serialization is working with it -- even though technically they should be objects.

As this change will only affect npm users and the correct "typing" is really Tags anyway along with the next release being 4.1.0 its a good time to break anyone how might be trying to pass an array (which they shouldn't).

I've already checked the 1DS code with this change and it complies as well.

Copy link
Collaborator

@MSNev MSNev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of references the tags[] in the API.md

And can you also put a placeholder in the RELEASES.md so that we don't forget about this potential breaking change. I'll update the title of this PR

@MSNev MSNev changed the title [main] [BUG] ITelemetryItem uses intersection type instead of union type for tags property #2258 [main] [Possible Break] Removed Tags[] from ITelemetryItem as this was breaking later versions of TypeScript by using the intersection type instead of union type for tags property #2258 Feb 12, 2024
@siyuniu-ms siyuniu-ms merged commit b5b9eea into main Feb 12, 2024
8 checks passed
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 this pull request may close these issues.

None yet

2 participants