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

[dart:js_interop] ExternalDartReference should have a generic argument. #55536

Closed
ykmnkmi opened this issue Apr 22, 2024 · 2 comments
Closed
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop

Comments

@ykmnkmi
Copy link
Contributor

ykmnkmi commented Apr 22, 2024

// There are also JS typed arguments and fields, used on the JS side, that accept null values.
extension type AppProperties._(JSObject _) implements JSObject {
  // So, I can't use checks like this
  factory AppProperties({void Function(({String text}))? message, int? optional}) {
    if (optional == null) {
      return AppProperties.__(message?.toExternalReference);
    }

    return AppProperties.__(message?.toExternalReference, optional: optional.toJS);
  }


  // No type check, can pass reference to anything.
  external factory AppProperties({ExternalDartReference? message});

  @JS('message')
  external ExternalDartReference? _message;

  void Function(({String text}))? get message {
    return _message?.toDartObject as void Function(({String text}))?;
  }
}
@nshahan nshahan added web-js-interop Issues that impact all js interop area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels Apr 22, 2024
@srujzs srujzs self-assigned this Apr 22, 2024
@srujzs
Copy link
Contributor

srujzs commented Apr 22, 2024

It's also useful to avoid the need for the user to do the cast. I think it's still new enough that the generic can be added fairly easily.

@srujzs
Copy link
Contributor

srujzs commented May 16, 2024

There are two options we can take here when making this generic. Both would update the representation type of ExternalDartReference in the JS compilers to T instead of Object.

  1. Make it generic but keep the bound as T extends Object. The existing members toExternalReference and toDartObject can be updated like:
extension ObjectToExternalDartReference<T extends Object> on T {
  external ExternalDartReference<T> get toExternalReference;
}
extension ExternalDartReferenceToObject on ExternalDartReference<T> {
  external T get toDartObject;
}

This unfortunately is a breaking change in at least one place (see #53711). We could possibly avoid that breaking change by adding a new member e.g. toExternalReferenceWithT and keeping the old one the same (let's call this option 1b), but I don't love that.

  1. The other alternative is making the bound T extends Object? and doing the same as all the above. This would mean a non-nullable ExternalDartReference could have a null representation field. It also means ExternalDartReference can no longer implement Object. This option would also be a breaking change, but would make working with arbitrary generic types T easier.

So, it seems like a breaking change is inevitable unless we do 1b, but I think that's okay since it looks like you're the only user outside of the SDK. :) FYI, it looks like you're also interested in Signals, so I'd look at some of the other issues @leonsenft filed to address some gaps of ExternalDartReference if you're curious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop
Projects
Status: Done
Development

No branches or pull requests

3 participants