Skip to content

Conversation

@sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Jul 8, 2025

There are some tests in this PR that check the @JSExport annotation when applied to extension types and extensions. The current behavior is that this annotation is a no-op when applied to an extension type or extension, and it produces a compile-time error when a member of an extension type or extension is annotated with this annotation. The tests are written accordingly. Please review.

@sgrekhov sgrekhov requested review from eernstg and srujzs July 8, 2025 13:37
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Commented on a bunch of typos. Awaiting further comments.

}

void main() {
var jsET = createJSInteropWrapper<C>(ET(C()));
Copy link
Member

Choose a reason for hiding this comment

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

This actually eliminates the extension type entirely, because createJSInteropWrapper<C> expects an actual argument of type C, and ET is a subtype of C so that's fine.

Does it work if it is invoked as follows?:

  var jsET = createJSInteropWrapper<ET>(ET(C()));

This could actually yield a JavaScript object with members etMethod, etGetter, etSetter, and c which would invoke the extension type members with the underlying C object as the representation object.

Alternatively, it could be rejected as an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it work if it is invoked as follows?:

 var jsET = createJSInteropWrapper<ET>(ET(C()));

No, it's an error. Error: Type argument 'ET' needs to be an interface type.. That's why I had to use class C as a type argument.

Copy link
Member

Choose a reason for hiding this comment

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

OK, but given that there is absolutely no difference between an instance of C and an instance of C which has been given the static type ET and upcasted to C, I'm not convinced that the use of ET shows anything. ;-)

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Thank you! Typos fixed. PTAL.

}

void main() {
var jsET = createJSInteropWrapper<C>(ET(C()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it work if it is invoked as follows?:

 var jsET = createJSInteropWrapper<ET>(ET(C()));

No, it's an error. Error: Type argument 'ET' needs to be an interface type.. That's why I had to use class C as a type argument.

Copy link

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

LGTM % my comments.

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Updated to expect warnings. Also added SDK issue numbers. PTAL.

@sgrekhov sgrekhov requested a review from eernstg July 9, 2025 08:09
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

LGTM.

@eernstg eernstg merged commit 040e206 into dart-lang:master Jul 9, 2025
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jul 11, 2025
2025-07-10 sgrekhov22@gmail.com dart-lang/co19#3180. Add JSExport tests. Part 3. (dart-lang/co19#3252)
2025-07-09 sgrekhov22@gmail.com dart-lang/co19#3180. Add JSExport tests. Part 2. (dart-lang/co19#3251)
2025-07-08 sgrekhov22@gmail.com dart-lang/co19#3180. Add additional `NullRejectionException` test (dart-lang/co19#3249)
2025-07-08 sgrekhov22@gmail.com dart-lang/co19#3180. Add JSExport tests. Part 1. (dart-lang/co19#3250)
2025-07-04 sgrekhov22@gmail.com Fixes dart-lang/co19#3245. Fix int to double conversion in jsNumber_t01.dart (dart-lang/co19#3248)
2025-07-04 sgrekhov22@gmail.com dart-lang/co19#3180. Add `NullRejectionException` tests (dart-lang/co19#3247)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try,dart2js-minified-linux-d8-try
Change-Id: I4b44a79d210d19153a464a9eadb86996d2876cf6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/439840
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Erik Ernst <eernst@google.com>
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