a14n / dart-google-maps

A library to use Google Maps JavaScript API v3 from Dart scripts.
Other
125 stars 63 forks source link

Why the most GMap properties and methods implemented as extension? #94

Closed alexd1971 closed 3 years ago

a14n commented 3 years ago

js-interop with package:js doesn't allow to have method implementations on @JS() annotated class. Using extensions is the only way I found to avoid wrapping every Js object with a Dart object.

There was an ambitious plan to improve js-interop but since @jmesserly left google I haven't seen a lot of change regarding this issue.

@sigmundch is there any plan to allow real implementation in @JS annotated classes ? For now I use a generator to get:

@JS('google.maps.LatLng')
class LatLng {
  external LatLng(num lat, num lng, [bool noWrap]);

  external bool equals(LatLng other);
  external String toString();
  external String toUrlValue([num precision]);
}

extension LatLng$Ext on LatLng {
  num get lat => _lat();
  num get lng => _lng();
  num _lat() => callMethod(this, 'lat', []);
  num _lng() => callMethod(this, 'lng', []);
}

but it would be better to be able to write directly:

@JS('google.maps.LatLng')
class LatLng {
  external LatLng(num lat, num lng, [bool noWrap]);

  external bool equals(LatLng other);
  external String toString();
  external String toUrlValue([num precision]);

  num get lat => _lat();
  @JS('lat')
  external num _lat();

  num get lng => _lng();
  @JS('lng')
  external num _lng();
}

especially from a test point of view if I need to mock LatLng.lat(not possible with extension).

alexd1971 commented 3 years ago

In previous version I could inherit GMap with it's implementation of methods. Now it seems to be not possible.

sigmundch commented 3 years ago

@a14n - thanks for the question.

The short answer is no. Currently our plans are shifting towards a more static approach than what we currently have.

One important reason we are pursuing this direction is that supporting interfaces/virtual dispatch comes at a very high cost. First, we need to distinguish instances of js-interop types at runtime, but don't reify enough type information to be able to do so properly. Adding that reification would be too expensive (e.g. if we were to wrap instances like the old dart:js used to do) and risk breaking composition (e.g. if two libraries happen to define interfaces for the same underlying JavaScript type). Second, even though today we declare them as classes, js-interop types behave differently and those small differences are often surprising and counter intuitive. Our goal for this year is to simplify this story and get to a point where JS-interop is more idiomatic and easier to use.

Jenny's proposal actually is similar to our current plans - in her proposal the idea was in fact to use static extension methods as well. At the time we didn't have them in the language, so she initially proposed using @sealed annotations as a syntactic sugar until the new syntax was added to Dart. So the examples you have above are honestly approaching the direction we are heading into.

Short term, the team is focusing on improve js_util methods so that callMethod, getProperty, etc (like in your example) behave just like the external declarations in terms of code quality and performance. We are also exploring generalizing the idea of extension methods to declare extension types, so that we may get a more succinct syntax to write everything in a single declaration. We will also be looking at simplifying how to create mocks easily, most likely by making mocks also JS-interop declarations that export mock methods from Dart.

Hope this clarify things a bit

a14n commented 3 years ago

@sigmundch - thanks for your answer.

We are also exploring generalizing the idea of extension methods to declare extension types, so that we may get a more succinct syntax to write everything in a single declaration.

Could you show me how the above LatLng declaration would look like with this new syntax?

We will also be looking at simplifying how to create mocks easily, most likely by making mocks also JS-interop declarations that export mock methods from Dart.

The issue with the current solution I used in the 4.0.0 (class + extension) is that the interface of LatLng is not what it should be:

// interface of LatLng
class LatLng {
  bool equals(LatLng other);
  String toString();
  String toUrlValue([num precision]);

  // the 2 getters below are defined in the extension and are not part of the interface
  // num get lat;
  // num get lng;
}

and it's impossible to mock LatLng to test:


num computeLatDelta(LatLng o1,LatLng o2) => (o1.lat - o2.lat).abs();
sigmundch commented 3 years ago

We are not yet finalized on the syntax, but we are currently working with a couple proposals for extension type sand zero-cost wrapper types.

The idea is that all JS-interop values at runtime are considered being a single type, say "JsObject", but all the JS-interop interactions are expressed in terms of static extensions. Unlike static extension methods, these extension types have a name and can be used as a type. Unlike regular classes, the language recognizes that these are actually views of an underlying type, so some restrictions are understood at the Dart level as first class (for example, the fact that putting js-interop types in type parameters is likely not reified as one would expect).

For example, using the syntax from the wrapper types proposal:

@JS()
class A {}

may become:

newtype A wraps JsObject {
}
@JS()
class A {}
extension A$ on A {
  get foo => callMethod(this, 'foo', [])
}

may look like:

@JS()
newtype A wraps JsObject {
  get foo => callMethod(this, 'foo', []);
}

LatLng may look like this:

newtype LatLng wraps JsObject {
  external bool equals(LatLng other);
  external String toUrlValue([num precision]);
  num get lat => callMethod(this, 'lat', []); // I can imagine we'd support the @JS/external rename syntax here too.
  num get lng => callMethod(this, 'lng', []);
}

A LatLng mock may look like this:

newtype LatLngMock wraps JsObject {
   factory LatLngMock({
      required num Function() lat,
      required num Function() lng,
   }) {
    return JsObject({
        'lat': allowInterop(lat),
        'lng': allowInterop(lng),
   });
  }
}

Because LatLngMock and LatLng both are extension types of the same underlying type, they are assignable, so you can pass them to computeLatDelta.

I didn't realize this on my earlier message, but we may be able achieve this kind of mocking with today syntax too. Since the beginning dart2js can't distinguish js-inteorp types (you could always cast any js-interop type to any other) and in the last couple months we also landed changes in DDC to make this consistent with dart2js's semantics. So you may be able to do something among these lines:

@JS()
@anonymous
class LatLngMock {
   external LatLngMock({Function lat, Function lng});
}

....

var x = LatLngMock({lat: allowInterop(() => 1), lgn: allowInterop(() => 2));
LatLng y = x as LatLng; // this will succeed, even without an "implements" clause in the declaration.
print(y.lat); // extension is now available, statement prints 1

If code works by shifting everything to static extension methods (except a constructor), then we'll be able to model it as an extension type going forward. We are at this time running experiments with internal code to ensure this is a viable approach.

a14n commented 3 years ago

@sigmundch - thanks a lot for all those informations!