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

make toggleAttribute match with native signature #5372

Merged
merged 1 commit into from
Oct 18, 2018

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Oct 11, 2018

Fixes #5364, again.

This solution is needed instead of #5371 as that would break TS definitions for consumers using TS <3.1.

I had to use arguments (unfortunately) because otherwise the function signature does not match the JSDoc, which seems more confusing than a a little arguments check.

This produces correct types which are identical to the native toggleAttribute, while still supporting our third argument behind the scenes in JS.

cc @TimvdLippe
Supersedes and closes #5371

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

SFTM. I will let @keanulee review as well.

@keanulee
Copy link
Contributor

This seems fine to me. @azakus you think this will be okay for internal users?

@43081j
Copy link
Contributor Author

43081j commented Oct 15, 2018

Any chance we can get this merged?

Also, does anyone know what the plan is for next release so i have a rough idea of when we'll get this? (the next time we tag & npm publish it)

@keanulee
Copy link
Contributor

cc @rictic @aomarks do you think this will be okay for internal users?

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Yeah, I don't see anything that should be a problem for internal users.

We're still on TS <3.1 internally too, so that's a bonus.

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

Successfully merging this pull request may close these issues.

Typescript 3.1 - compile error. toggleAttribute type conflict in LegacyElementMixin
5 participants