`@JS()` doesn't support or fail on `external List<NotDynamic> get` #34195

jakemac53 · 2018-08-20T13:49:56Z

Original issue file by @jackd dart-lang/build#1771

Dart2js produces code throwing TypeErrors when using wrapped javascript which is known to return typed arrays. ddc code runs fine. cast on the output of dart2js fixes the issue, but is somewhat unsatisfying.

Repo demonstrating the issue here (including all code below).

Result of running ddc js:
woof, moo

Result of running dart2js js:
Uncaught Error: TypeError: Closure 'minified:by': type '(minified:F) => String' is not a subtype of type '(dynamic) => String'

farm.js

function Animal(sound) {
  this._sound = sound
}

Animal.prototype.hello = function() {
  return this._sound;
}

function Farm() {
  this._animals = [new Animal('woof'), new Animal('moo')];
}

Farm.prototype.animals = function() {
  return this._animals;
}

farm.dart

@JS()
library farm;

import 'package:js/js.dart';

@JS()
class Animal {
  external factory Animal(String sound);
  external String hello();
}

@JS()
class Farm {
  external factory Farm();
  external List<Animal> animals();
}

index.dart

import 'farm.dart';

void main() {
  var animals = new Farm().animals();
  // animals = animals.cast<Animal>();  // fixes errors in dart2js
  print(animals.map<String>((a) => a.hello()).join(', '));
}

index.html

<!DOCTYPE html>
<html>
<head>
    <script src="./farm.js"></script>
    <script defer src="./index.dart.js"></script>
</head>
<body></body>
</html>

The text was updated successfully, but these errors were encountered:

jakemac53 · 2018-08-20T13:50:40Z

@jacob314 @rakudrama

matanlurey · 2018-08-20T21:38:20Z

The following code is invalid in Dart 2:

@JS()
class Farm {
  external factory Farm();
  external List<Animal> animals();
  //            ^^^^^^^^^^^^^^
}

In JavaScript-compiled Dart, the backing implementation of List is Array, which of course, does not have generic type arguments - so it is considered a List<dynamic> in Dart2JS and DDC. In Dart 1 the following was valid:

List<Animal> animals = <dynamic>[];

This of course, is not valid in Dart 2.

We'll have to either:

Make it a compile-time error or warning to assume reified generic types on @JS() interfaces.
Add automatic List.from or <List>.cast's to all generic lists on @JS() interfaces.

You can see some documentation here:
https://github.com/matanlurey/dart_js_interop#generic-type-arguments

jackd · 2018-08-20T22:48:42Z

Thanks for the documentation repo - I can confirm using --omit-implicit-checks resolves the issue.

Can I ask what the argument is for the first option over the second? As I understand it, there's no way of getting a typed list back from javascript, so external List<Animal> animals() unambiguously communicates a desire to cast the returned value. Are there implementations of casting that are better in certain circumstances than others? If there are, whatever the default behaviour is, it could always be turned off by leaving off the generic types in the wrapper.

I'd rather not have to write

class WrappedFarm {
  final Farm _farm;
  WrappedFarm(this.farm);
  List<Animal> animals() => _farm.animals().cast<Animal>();
  // other generic returned types

for every class in something like this, which is itself already a wrapper around another source (even if that wrapper is generated automatically).

matanlurey · 2018-08-20T23:15:41Z

Can I ask what the argument is for the first option over the second?

Having to change 2 compilers in a potentially non-trivial way to add code that isn't always necessary, I guess. It's not really clear to me what or how @JS()-based interop will evolve to better support Dart 2 yet, so those were just ideas from me, not authoritative by any means :)

Unambiguously communicates a desire to cast the returned value.

Both carry an overhead too. Technically List<Animal>.from is strictly speaking better (see avoid using cast in effective Dart), but it makes a copy, which isn't what you always want.

I believe this is an issue with DDC as well, not just Dart2JS, so re-categorizing.

srawlins · 2018-10-31T19:40:21Z

I think at this point I would prefer option 2, automatic List.from.

Without this change, if we had to wrap ourselves with List.from(), I think the best way to write js-interop APIs is like so:

@JS('foo.bar.getStuff')
external List<dynamic /* num */> _getStuff();
List<num> getStuff() => _getStuff();

@JS('foo.bar.getThings')
external List<dynamic /* num */ > _getThings(Something a, [String b, String c]);

List<num> getThings(Something a, [String b, String c]) {
  if (b == null) {
    return _getThings(a);
  } else {
    return c == null ? _getThings(a, b) : _getThings(a, b, c);
  }
}

What a joy that is! (The proper arity is required for undefined checks in JS, correct?)

And I'm not sure how you would write a similar wrapper for a class:

@JS('foo.bar.Bar')
class Bar {
  external factory Bar();
  // Can you wrap this with `@JS()` as well??? 
  // So that you could write this as _getList()?
  external List<num> getList();
}

srawlins · 2018-11-01T20:36:11Z

Ah, I see @jackd actually already gave a little solution to the class issue. Here's a slightly different take, with some visibility benefits. Any code outside this library only sees the one class, and is unaware that it's a wrapper. Let's say Farm's constructor also takes a String name:

@JS('Farm')
class _Farm {
  external factory _Farm(String name);
  external List<Animal> animals();
}

class Farm {
  final _Farm _jsFarm;

  factory Farm(String name) {
    var jsFarm = _Farm(name);
    return Farm._(jsFarm);
  }

  Farm._(this._jsFarm);

  List<Animal> animals() => List<Animal>.from(_jsFarm.animals());
}

jackd · 2018-11-02T00:24:52Z

My biggest issue here isn't so much the need for additional code, but the inconsistency between DDC and dart2js.

Currently, external List<Animal> animals() -always- throws when built using dart2js without --omit-implicit-checks. There's no ambiguity about the desired output of the code - just in the desired implementation dart2js should use (cast or from). Personally, I feel any default implementation would be preferable to an always-throw, since users can override the behaviour by writing their own implementation like @srawlins (with _Farm.animals returning an untyped List). This would make DDC and dart2js consistent without limiting control over the implementation in performance-critical code.

matanlurey · 2018-11-02T01:42:26Z

To be clear, both DDC and Dart2JS always throw. Dart2JS with --omit-implicit-checks is not intended to fix bugs, but rather to be used for well-formed applications that work without --omit-implicit-checks.

jackd · 2018-11-02T02:07:00Z

Confirmed, sorry for the confusion. That comment was based on my experience with 2.0 dev release.

jmesserly · 2018-11-15T21:01:52Z

I'll add a note to #35084 so we handle this correctly, and merge this bug into that one. Thanks so much for reporting! I'm working on (what I hope will be) big improvements to JS interop.

TL;DR -- I want to support the initial example. Arrays created in JS will not have a Dart reified type, so we'll allow them to be cast to any list type.

jakemac53 added the web-dart2js label Aug 20, 2018

jakemac53 mentioned this issue Aug 20, 2018

dart2js TypeErrors on typed javascript-wrapped arrays dart-lang/build#1771

Closed

6 tasks

matanlurey added web-js-interop Issues that impact all js interop and removed web-dart2js labels Aug 20, 2018

matanlurey changed the title ~~dart2js TypeErrors on typed javascript-wrapped arrays~~ @JS() doesn't support or fail on external List<NotDynamic> get Aug 20, 2018

jmesserly added the closed-duplicate Closed in favor of an existing report label Nov 15, 2018

jmesserly closed this as completed Nov 15, 2018

jmesserly mentioned this issue Nov 15, 2018

Improve JS interop #35084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`@JS()` doesn't support or fail on `external List<NotDynamic> get` #34195

`@JS()` doesn't support or fail on `external List<NotDynamic> get` #34195

jakemac53 commented Aug 20, 2018

jakemac53 commented Aug 20, 2018

matanlurey commented Aug 20, 2018 •

edited

Loading

jackd commented Aug 20, 2018

matanlurey commented Aug 20, 2018

srawlins commented Oct 31, 2018

srawlins commented Nov 1, 2018

jackd commented Nov 2, 2018

matanlurey commented Nov 2, 2018

jackd commented Nov 2, 2018

jmesserly commented Nov 15, 2018

@JS() doesn't support or fail on external List<NotDynamic> get #34195

@JS() doesn't support or fail on external List<NotDynamic> get #34195

Comments

jakemac53 commented Aug 20, 2018

jakemac53 commented Aug 20, 2018

matanlurey commented Aug 20, 2018 • edited Loading

jackd commented Aug 20, 2018

matanlurey commented Aug 20, 2018

srawlins commented Oct 31, 2018

srawlins commented Nov 1, 2018

jackd commented Nov 2, 2018

matanlurey commented Nov 2, 2018

jackd commented Nov 2, 2018

jmesserly commented Nov 15, 2018

`@JS()` doesn't support or fail on `external List<NotDynamic> get` #34195

`@JS()` doesn't support or fail on `external List<NotDynamic> get` #34195

matanlurey commented Aug 20, 2018 •

edited

Loading