[S] Change in dart/sdk[main]: [dart:js_interop] Support extension types on generic type parameters
Arnav Arora (Gerrit)
Arnav Arora voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Srujan Gaddam (Gerrit)
Srujan Gaddam voted and added 4 comments![]( "Open in Gerrit")
Votes added by Srujan Gaddam
| Code-Review | +1 |
4 comments
Change LGTM, just some comments on testing.
final interopType = getCoreInteropType(onType.nonTypeParameterBound);nit: This support the legacy `@staticInterop` types as well. Maybe worth adding a test where the extension is on a type parameter that is bound to a `@staticInterop` type as well?
V extends JSObjectMaybe use a user-defined extension type here to test with as the type parameter bound? Like `ExtensionType` in tests/lib/js/static_interop_test/extension_type/external_extension_member_test.dart?
extension SelfReferenceExtension<T extends JSObject> on T {
external T? get selfReference;
}
extension ChainedSelfReferenceExtension<
T extends U,
U extends V,
V extends JSObject
>
on T {
external T? get chainedSelfReference;
}Consider putting these in tests/lib/js/static_interop_test/extension_type/external_extension_member_test.dart instead and use this test just for a type parameter bound to a `@staticInterop` type test instead.
Related details
- Arnav Arora
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arnav Arora (Gerrit)
Arnav Arora voted and added 3 comments![]( "Open in Gerrit")
Votes added by Arnav Arora
| Commit-Queue | +1 |
3 comments
final interopType = getCoreInteropType(onType.nonTypeParameterBound);nit: This support the legacy `@staticInterop` types as well. Maybe worth adding a test where the extension is on a type parameter that is bound to a `@staticInterop` type as well?
Done
Maybe use a user-defined extension type here to test with as the type parameter bound? Like `ExtensionType` in tests/lib/js/static_interop_test/extension_type/external_extension_member_test.dart?
Done
extension SelfReferenceExtension<T extends JSObject> on T {
external T? get selfReference;
}
extension ChainedSelfReferenceExtension<
T extends U,
U extends V,
V extends JSObject
>
on T {
external T? get chainedSelfReference;
}Consider putting these in tests/lib/js/static_interop_test/extension_type/external_extension_member_test.dart instead and use this test just for a type parameter bound to a `@staticInterop` type test instead.
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Srujan Gaddam (Gerrit)
Srujan Gaddam added 3 comments![]( "Open in Gerrit")
{Very tiny nit, but do we need the extra scope? Pro is that it breaks apart the multiple tests better but con is that it's inconsistent with the rest of the test.
extensionObj as JSObject;nit: Don't need the cast because the type implements `JSObject`.
var obj = StaticInteropClass();This doesn't exist in the JS `eval` code, so I expect this test will fail.
Related details
- Arnav Arora
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arnav Arora (Gerrit)
Arnav Arora voted and added 3 comments![]( "Open in Gerrit")
Votes added by Arnav Arora
| Commit-Queue | +1 |
3 comments
Very tiny nit, but do we need the extra scope? Pro is that it breaks apart the multiple tests better but con is that it's inconsistent with the rest of the test.
Done
nit: Don't need the cast because the type implements `JSObject`.
Done
This doesn't exist in the JS `eval` code, so I expect this test will fail.
- Nate Biggs
- Srujan Gaddam
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |