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.22k stars 1.57k forks source link

Add a "call with new" option to JS interop function/method bindings #43058

Open yjbanov opened 4 years ago

yjbanov commented 4 years ago

It is common in JavaScript to nest constructors under a namespace. For example, CanvasKit has a canvasKit.SkPictureRecorder and canvasKit.SkPaint constructors. These constructors must be called with the new keyword. However, Dart's JS interop has no way to instruct the compiler to call these functions with new.

The feature request is to add a way to tell DDC and dart2js to use the new keyword.

One option is to add a new annotation @callWithNew used like this:

@JS()
class CanvasKit {
  @callWithNew
  external SkPictureRecorder MakePictureRecorder();
}

Another option is to add an optional boolean parameter to the @JS annotation:

@JS()
class CanvasKit {
  @JS(callWithNew: true)
  external SkPictureRecorder MakePictureRecorder();
}
natebosch commented 4 years ago

Is the goal that you have JS that looks like new CanvasKit.MakePictureRecorder()?

We don't support it today, but my first idea for how to address this use case would be to support renames for factory constructors.

@JS()
class SkPictureRecorder {
  @JS('CanvasKit.MakePictureRecorder')
  external factory SkPictureRecorder()
}

I'm not sure if there are blockers for allowing renames on factory constructors like there are blockers for renames of instance members...

sigmundch commented 4 years ago

@yjbanov - can you clarify, is part of the issue here that you'd like the class to remain @anonymous in Dart? Or are there issues if you tried to access the constructor from the window object?

I ask because for non-anonymous classes today you can give the class a different name, and both compilers use the name in the @JS annotation of the class as the constructor path. For example: new SkPictureRecorder() is compiled to new canvasKit.MakePictureRecorder() in this code:

@JS('CanvasKit.MakePictureRecorder')  // the name matches the path to the constructor invoked with `new`
class SkPictureRecorder {             // the class name can be different in Dart
  external factory SkPictureRecorder();
}

For anonymous classes created with a constructor we'd need something more. I like @natebosch's suggestion as a short term solution. Long term, once we fix DDC to behave like dart2js when it comes to is/as checks, I believe you'll no longer need to use anonymous as you had to do recently.

yjbanov commented 3 years ago

I don't think this is about @anonymous, or maybe it is and I'm just not sure how. The issue is that you have a reference to a JS object and that object provides a method that requires that it's called with new and there's no way to tell JS-interop to call that method with the new keyword.

Both @natebosch's and @sigmundch's ideas assume there's some top-level variable available at the global scope named CanvasKit that would make the expression new CanvasKit.MakePictureRecorder() valid, but there's no such variable. All you have is a reference to an object of type/prototype CanvasKit, and that only happens to be the case in the CanvasKit API. More generally, you can get any object from JS, anonymous or not, with a method on it that requires that it's called with new. You don't even need an object, it can be any function reference that requires new because the body of any function/method may need access to this.

Here are some possible scenarios (it's more interesting when obj, bar2, and baz are not global variables):

function foo() {
  if (new.target) {
    console.log('foo called with new');
  } else {
    console.log('foo not called with new');
  }
}

let obj = {
  'bar': function () {
    if (new.target) {
      console.log('bar called with new');
    } else {
      console.log('bar not called with new');
    }
  },
  'bar2': function() {
    return {
      'bar3': function () {
        if (new.target) {
          console.log('bar3 called with new');
        } else {
          console.log('bar3 not called with new');
        }
      }
    };
  }
};

class Baz {
  qux() {
    if (new.target) {
      console.log('qux called with new');
    } else {
      console.log('qux not called with new');
    }
  }
}

foo();
new foo();

obj.bar();
new obj.bar();

obj.bar2().bar3();
new (obj.bar2().bar3)(); // extra parens to fix operator precedence

// Now let's say to want to keep a reference on bar2 and call bar3 method
// repeatedly. This is not the same as executing `new obj.bar2().bar3()`
// repeatedly, because the latter calls `bar2()` multiple times.
let bar2 = obj.bar2();
for (let i = 0; i < 10; i++) {
  new bar2.bar3();
}

let baz = new Baz();
baz.qux();
// new baz.qux();  // ILLEGAL
sigmundch commented 3 years ago

interesting - a couple questions:

The concern I have with annotating an instance member with a calling convention, is that we can only make that work well if the target is statically known. Dynamic calls on that method will not know what to do. The one way we can address it is to only allow such annotation on external static methods or external static extension methods. We don't have support for the latter yet, but do plan to add it in the near future (#44057).

Shorter term, maybe we can provide some helper methods in js_util as an alternative? That would allow you to choose to use new on each callsite if you need it. In fact, it seems like js_util.callConstructor was added with a similar goal in mind, but I don't believe we have tested it in scenarios like the one you describe. I just tried it with a small example, and it looks promising. Would that do what you need?

The one caveat is that callConstructor is a bit more convoluted when you pass arguments, so we need to play around with it, and may be worth tuning it or providing something else to make it smoother.

yjbanov commented 3 years ago
  • (a) are there cases where you'd need to call the method both with and without new?

Not in my case, but JS allows it, so I guess Dart JS-interop should support it too. I think this could be supported by allowing renames, e.g.:

@JS()
class CanvasKit {
  @callWithNew
  external SkPictureRecorder MakePictureRecorder();

  /// Even though the Dart method is called `MakePictureRecorderWithoutNew`, the `@JS` annotation
  /// rewrites it to call `MakePictureRecorder` instead.
  @JS('MakePictureRecorder')
  external SkPictureRecorder MakePictureRecorderWithoutNew();
}
  • (b) how much is the problem access to this or having a this instance with a prototype based on bar3's constructor? In other words, are there cases where it would be enough to use bind and use a different object instance as this?

I'm not sure what would be sufficient, but all combinations are possible, each distinct from others:

const obj = {
  x: 42,
  func: function() {
    console.log(`  called ${ new.target ? 'with' : 'without' } new.`);
    console.log(`  this is ${ this }${ this.x ? ' with ' + this.x : '' }.`);
  }
};

console.log('Unbound tear-off');
const unboundFunc = obj.func;
unboundFunc();

console.log('Bound tear-off');
const boundFunc = unboundFunc.bind({ x: 3.14 });
boundFunc();

console.log('Plain invocation');
obj.func();

console.log('Invocation with new');
new obj.func();

Output:

> "Unbound tear-off"
> "  called without new."
> "  this is [object Window]."
> "Bound tear-off"
> "  called without new."
> "  this is [object Object] with 3.14."
> "Plain invocation"
> "  called without new."
> "  this is [object Object] with 42."
> "Invocation with new"
> "  called with new."
> "  this is [object Object]."

Shorter term, maybe we can provide some helper methods in js_util as an alternative? That would allow you to choose to use new on each callsite if you need it. In fact, it seems like js_util.callConstructor was added with a similar goal in mind, but I don't believe we have tested it in scenarios like the one you describe. I just tried it with a small example, and it looks promising. Would that do what you need?

I think a less convenient escape hatch for uncommon situations would be fine, so long as it performs well.

sigmundch commented 3 years ago

I think a less convenient escape hatch for uncommon situations would be fine, so long as it performs well.

Sounds good - let us know if the current callConstructor is not enough. It appears like it may do already what you need.

rodydavis commented 2 years ago

I just tried the example snippet and it does not seem to be working with flutter 3 on the web.