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

Don't emit interface mixins #90

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Oct 27, 2023

They should be added to the interfaces they are mixed into. This allows for external static members to use the right nominal type. This CL also adds some logic to handle name collisions between instance and static operations, which is now needed for exactly one pair of operations: Response.prototype.json and Response.json. Also enables asserts in the translator.

#56

@srujzs srujzs requested a review from sigmundch October 27, 2023 01:24
Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

Uh. Doesn't this mean, in some cases, you won't be able to polymorph over similar types since they won't share a base type?

@kevmoo
Copy link
Member

kevmoo commented Oct 27, 2023

Uh. Doesn't this mean, in some cases, you won't be able to polymorph over similar types since they won't share a base type?

Like you won't be able to make a function that takes GenericTransformStream as a type...

@srujzs
Copy link
Contributor Author

srujzs commented Oct 27, 2023

That's true, but that's intentional, since they're not supposed to be used as types.

https://webidl.spec.whatwg.org/#idl-interface-mixins

Note that unlike interfaces or dictionaries, interface mixins do not create types.

Do you have an example where they are that I overlooked? GenericTransformStream isn't used as a type in the IDL afaict. If they are used as types, then one option is to just move the static members to the mixing-in class and keep the instance members and mixin interface around.

They should be added to the interfaces they are mixed into. This
allows for external static members to use the right nominal type.
This CL also adds some logic to handle name collisions between
instance and static operations, which is now needed for exactly
one pair of operations: Response.prototype.json and Response.json.
@kevmoo
Copy link
Member

kevmoo commented Oct 30, 2023

Do you have an example where they are that I overlooked?

I can imagine places where writing helpers/extensions on the shared types could be nice. I mean, we can always go back to these later. This is ABSOLUTELY a breaking change, though.

...I mean, if folks are actually using these types already. 🤷‍♂️

@srujzs
Copy link
Contributor Author

srujzs commented Oct 30, 2023

Searching GitHub gave me no results for these mixin types being used with package:web, but that isn't too surprising. I've added this change to the changelog, and if we change our minds later about wanting these types to be emitted, we can go the route of comment#5..

@srujzs srujzs merged commit 179a959 into dart-lang:main Oct 30, 2023
6 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.

3 participants