Make JSExportedDartFunction generic and add a toJSGeneric function

[srujzs]

It is useful to know the type of the Dart function that is now being wrapped by a JS function. Imagine an interface like:

@JS()
external set callback(JSFunction f);

where callback is called later at some point. The external member has no way of validating that f is a function that will work with the inputs that are passed and produces an output that is expected. If f wraps a function with the wrong type, this results in a runtime error later. In order to have some better static checking, we essentially need to duplicate the member:

@JS('callback')
external set _callback(JSFunction f);
set callback(void Function(int) f) => _callback = f.toJS;

Instead, we can improve the static checking here by making JSExportedDartFunction generic with a bound of Function and exposing a member that produces the generic type e.g.

JSExportedDartFunction<T> toJSGeneric<T extends Function>(T t) => t.toJS as JSExportedDartFunction<T>;

which would then result in code like:

@JS()
external set callback(JSExportedDartFunction<void Function(int)> f);

Granted, unlike with the example with two setters, this doesn't stop users from casting one JSExportedDartFunction to another JSExportedDartFunction and basically lie about the generics, but this makes it statically obvious what JS is expecting.

[srujzs]

cc @ditman because I know you came across this often with factories in Flutter.

[srujzs]

There is a downside here in that if we adopt this in package:web, it is a breaking change. It's also worth mentioning that if we expect a JSExportedDartFunction in package:web for all callbacks, that means non-exported functions are no longer accepted, which might not be what we want.

[ditman]

(This reminds me a little bit of the trick I used for some tests: .jsifyAs<T extends JSAny?>().)

I don't have a strong preference on this one, I'm getting used to using JS*types in all external definitions, and when I want more ergonomic/specific Dart types to split the definition and have just a delegated call with all .toJS/.toDart called internally.

For ergonomics, this is much nicer, but the more we hide the fact that we're interfacing with JS, the more cryptic errors may become in Wasm targets, maybe?

[srujzs]

For ergonomics, this is much nicer, but the more we hide the fact that we're interfacing with JS, the more cryptic errors may become in Wasm targets, maybe?

I don't think the runtime error would really change here. The generic on the extension type would be purely static (as it doesn't forward to the representation type).

But okay, I don't have a strong preference here either, so to the backlog it goes. The nice thing about splitting is you get useful runtime errors on the Function, so this is not strictly an improvement.

[AlexDochioiu]

@JS()
external set callback(JSExportedDartFunction<void Function(int)> f);

+1 for this approach over the two setters approach.

I expect the 2 setters approach could be problematic if you have something like:

@JS('removeListener')
external void _removeListener(JSFunction f);
@JS('addListener')
external void _addListener(JSFunction f);

void removeListener(void Function(int) f) => _removeListener(f.toJS);
void addListener(void Function(int) f) => _addListener(f.toJS);

This is what I think will happen. I did not test this so please correct me if my assumption is wrong:
Calling addListener and removeListener on the same dart function will yield a different instance of JSExportedDartFunction, causing the removeListener to do nothing.

---

Having a typed JSExportedDartFunction that gets passed directly (and not converted internally through another function from Dart) may make this mistake/oversight less likely to happen. (of course, it's still possible to happen if toJS is called twice)

[ditman]

This is what I think will happen. I did not test this so please correct me if my assumption is wrong.

That's exactly what you need today, yep. In this particular example, you're right that making the .toJS transparently is bad for how the JS API works, in that case, you shouldn't give users the dartified API IMO.

[srujzs]

Calling addListener and removeListener on the same dart function will yield a different instance of JSExportedDartFunction, causing the removeListener to do nothing.

Right. There's some logic on the JS backends to make sure you don't accidentally rewrap the same function, but that doesn't help here. In general, adding and removing listeners is wonky and I'd rather users try to use some of the Stream helpers in this package if possible.