dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

JS interop classes can not implement Iterable (dart2js) #30969

Open pulyaevskiy opened 7 years ago

pulyaevskiy commented 7 years ago

First encountered this in dart:js_util when used jsify(). It kept failing with a stack trace like this:

  NoSuchMethodError: method not found: 'map$1' (t2.map$1 is not a function)
  dart:js_util                              NodeClient._jsifyHeaders
  package:node_interop/http.dart 69:18      NodeClient.send
  package:http/src/base_client.dart 171:38  <fn>
  ===== asynchronous gap ===========================
  dart:_internal                            Object._asyncRethrow
  package:http/src/base_client.dart 151:3   <fn>
  ===== asynchronous gap ===========================
  dart:_internal                            Object._wrapJsFunctionForAsync
  package:http/src/base_client.dart 151:3   NodeClient._sendUnstreamed
  package:http/src/base_client.dart 56:5    <fn>
  ===== asynchronous gap ===========================
  dart:_internal                            Object._wrapJsFunctionForAsync
  test/http_test.dart 62:43                 main.<fn>.<fn>

Looking at implementation of jsify it checks if an object is Iterable and tries to call map() on it. This fails when running tests in node (pub run test --platform node).

Similar errors found in matcher package's prettyPrint() which uses similar checks.

rakudrama commented 7 years ago

String (JS String) is not Iterable.

pulyaevskiy commented 7 years ago

Thanks, I think you are right... I found the issue: never declare external JS class as implementing Iterable.

I declared Node's Buffer as:

@JS()
abstract class Buffer implements Iterable<int> {
  // etc...
}

Which is true. And it also was convenient because I could just pass it to new List.from(buffer) for instance.

This apparently resulted in common Interceptor for all other classes (JSBool, JSString,...) getting §isIterable: 1, which I think is the reason, but I don't know much about interceptors. Comparing different compiled outputs of dart2js gave me a hint though.

I removed implements Iterable<int> from Buffer for now, but curious if there is better/right way to achieve this?

Is it a bad practice to make JS classes implement something else?

rakudrama commented 7 years ago

This is appears to the the same issue as https://github.com/dart-lang/sdk/issues/26838 I'll let @jacob314 suggest advice.

pulyaevskiy commented 7 years ago

Just found another edge case. Implementing call() on a JS-annotated class adds $isFunction: 1 to Interceptor and breaks at least assert() checks (bool is treated as a function).

For instance, ExpressJS uses this for the main library object and Application object:

@JS()
abstract class Express {
  external Application call();
  // more methods here...
}

@JS()
abstract class Application {
  external void call(req, res, [next]);
}

void main() {
  Express express = require('express');
  var app = express(); // etc...
}

Note that I would be happy to help with fixing this issue but I'd appreciate some help in a form of:

As suggested in #26838 it seems like a solution would be to move those $isXXX flags to JavaScriptObject interceptor.

Looks like all my JS-annotated classes inherit from JavaScriptObject, while regular Dart classes inherit from Object, so it seems like a reasonable change... or is it?

cedx commented 6 years ago

@rakudrama If a JS string is not iterable, why this JS code works ?

for (let char of 'foo') console.log(char);

/// prints :
f
o
o

I'm OK with the fact that JS strings don't implement the Dart Iterable interface, but they can definitely be iterated. See : String.[@@iterator]()

I think that dart2js or @JS() should make Iterable JS classes implementing the Symbol.iterator property.