[web/interop] New JS interop extensions that provide `jsify()` / `dartify()`

mkustermann commented 8 months ago

Currently the dart:js_interop provides a jsify() method that is described as this:

extension NullableObjectUtilExtension on Object? {
  /// Converts a Dart object to the JavaScript equivalent if possible.
  ///
  /// Effectively the inverse of [JSAnyUtilityExtension.dartify], [jsify] takes
  /// a Dart object and recursively converts it to a JavaScript value. Only Dart
  /// primitives, [Iterable]s, typed lists, and [Map]s are supported.
  ///
  /// > [!NOTE]
  /// > Prefer using the specific conversion method like `toJS` if you know the
  /// > Dart type as this method may perform many type-checks.
  // TODO(srujzs): We likely need stronger tests for this method to ensure
  // consistency.
  external JSAny? jsify();
}

=> Based on this description it would seem one can only use .jsify() for things that can be encoded as json (i.e. maps, lists and primitives (including e.g. typed arrays))

But the implementation allows using arbitrary objects:

import 'dart:js_interop';
class Foo {}
main() {
  print(Foo());
  print(Foo().jsify().dartify());
}

On top of that, it behaves differently on dart2js vs dart2wasm:

% dart compile js test.dart
% third_party/d8/linux/x64/d8 out/ReleaseX64/dart-sdk/lib/_internal/js_runtime/lib/preambles/d8.js out.js
Instance of 'Foo'
Instance of 'Foo'

% dart compile wasm test.dart
% pkg/dart2wasm/tool/run_benchmark test.wasm                                                             
Instance of 'Foo'
{}

So I think we have to address at least those two aspects:

[ ] Implementation allows arbitrary objects <-> API docs say it doesn't
[ ] Behaves differently in dart2js vs dart2wasm

IMHO it would make sense to rethink the API itself:

[ ] Ensure we have the basic building blocks, so users can write their own recursive jsify() / dartify()
[ ] Do we actually want to provide an implementation in core libraries?
[ ] Can we separate Tree vs DAG handling (most uses will have trees in Dart already - so we pay an unnecessary high cost for DAG handling)
[ ] Can we restrict to pure JSON only (avoid overhead of 15+ is <X> / instance of <X> calls)?
[ ] Can we restrict to JSON only or should allow wrapped user objects (IMHO dangerous (e.g. postMessage(<obj>.jsify()) may fail in dart2wasm and do strange things in dart2js))
[ ] Should we require users to specify whether JS numbers that can be represented as int64 should be converted to int64 or not (see e.g. https://github.com/dart-lang/sdk/issues/55203)
[ ] Should we offer an option to throw if numbers loose precision when crossing boundary

/cc @srujzs @sigmundch

srujzs commented 8 months ago

I think the ability to call jsify on arbitrary Dart objects is a relic from dart:js_util. The above conversion is almost never what the user wants, and may lead to subtle bugs where users thought a different value was passed instead. I think we should prefer to instead throw if the object is not one of the allowed values.

Behaves differently in dart2js vs dart2wasm

We should double-check our tests to make sure we have good coverage here. Are you seeing this discrepancy even in the allowed types or just the disallowed types (arbitrary Dart objects)?

Ensure we have the basic building blocks, so users can write their own recursive jsify() / dartify()

I believe beyond the Map=>JS object and the Iterable=>JS array conversion, the rest already exist with the existing toJS conversions (the Iterable conversion only exists for Lists), but we can add the former two if we want. I'm not sure how useful they'll be as standalone conversions.

Do we actually want to provide an implementation in core libraries?

Yes, at least for dartify when you get back JSAnys from JS that could be several different values. Manually type-checking all the possible values is less desirable. There are likely some web APIs that benefit from jsify as well.

Can we separate Tree vs DAG handling (most uses will have trees in Dart already - so we pay an unnecessary high cost for DAG handling)

I can see this adding overhead if it's a large map. We could make this a named parameter e.g. isTree to avoid the excessive checking if users know better.

Can we restrict to pure JSON only (avoid overhead of 15+ is / instance of calls)?

We could speed-up the current is checks by just checking for is TypedData - not sure why we don't (maybe to be more explicit about which TypedData we support?).

Can we restrict to JSON only or should allow wrapped user objects (IMHO dangerous (e.g. postMessage(.jsify()) may fail in dart2wasm and do strange things in dart2js))

Can you specify what you mean by "wrapped user objects" and what "" here is?

Should we require users to specify whether JS numbers that can be represented as int64 should be converted to int64 or not Should we offer an option to throw if numbers loose precision when crossing boundary

Reading through the other thread, would the user ever want to lose precision here? Should we always throw? Or is checking whether we'd lose precision slow?

mkustermann commented 8 months ago

Are you seeing this discrepancy even in the allowed types or just the disallowed types (arbitrary Dart objects)?

I meant the discrepancy of instances of user-defined classes. We do have the number discrepancy as well, where in wasm one always gets doubles atm IIRC.

Yes, at least for dartify when you get back JSAnys from JS that could be several different values. Manually type-checking all the possible values is less desirable. There are likely some web APIs that benefit from jsify as well.

Even if one gets the dartify()ed object graph, one still has to manually type check/cast all the values, it's just on the dart types instead of the js types.

What I meant here is: If we have building blocks, we could have the extension e.g. in package:web or somewhere else. By having it in dart:js_interop and by having them under the very general dartify() / jsify() names we actively encourage users to use them (just too easy to use compared with .toJS* methods).

Can you specify what you mean by "wrapped user objects" and what "" here is?

Objects of user-defined classes (like (new Foo().jsify() in the example).

You mentioned this is unintentional in the first place (👍 ) and we should throw. If users need it for some esoteric use case, they can write their own recursive code for that (if we have the building blocks)

Reading through the other thread, would the user ever want to lose precision here? Should we always throw? Or is checking whether we'd lose precision slow?

We'd need to benchmark to see the impact. Seems to be also somewhat inconsistent atm for the explicit operations

for <jsNumber>.toDartInt we apparently do check
for <dartInt>.toJS we apparently do not check

srujzs commented 8 months ago

I meant the discrepancy of instances of user-defined classes. We do have the number discrepancy as well, where in wasm one always gets doubles atm IIRC.

Got it, disallowing user-defined classes should get us just to just the double case in terms of discrepancies.

What I meant here is: If we have building blocks, we could have the extension e.g. in package:web or somewhere else. By having it in dart:js_interop and by having them under the very general dartify() / jsify() names we actively encourage users to use them (just too easy to use compared with .toJS* methods).

This is a fair point. I have seen users leaning towards these functions and unintentionally introduing overhead, whereas if they knew the type or set of types the value could be, the conversion would be faster. If we added the conversion functions for Map and Iterable, maybe removing jsify/dartify makes sense. Slightly related: I've also seen feature requests for supporting JS Maps and Sets but the conversions for those types are not the same as what jsify/dartify do.

You mentioned this is unintentional in the first place (👍 ) and we should throw. If users need it for some esoteric use case, they can write their own recursive code for that (if we have the building blocks)

Agreed.

We'd need to benchmark to see the impact. Seems to be also somewhat inconsistent atm for the explicit operations

I think this is just oversight. The JS compilers don't have the same issue around losing precision, and the conversion to int case is much more common than losing precision. I presume the cost isn't high as the bottleneck will be the interop, but benchmarking makes sense.

srujzs commented 7 months ago

There's also one other discrepancy I noticed here we should fix:

https://github.com/dart-lang/sdk/blob/061004f83f2f606b5db9488ba50c478dcaa22ba4/sdk/lib/_internal/wasm/lib/js_util_patch.dart#L29

https://github.com/dart-lang/sdk/blob/061004f83f2f606b5db9488ba50c478dcaa22ba4/sdk/lib/_internal/js_shared/lib/js_util_patch.dart#L36

null returns two different values here. On DDC and dart2js, you get back null, and on dart2wasm, you get back a JSValue.

curt-weber commented 5 months ago

Not sure if this is the right place - but I found that dartify is different between dart2js and dart2wasm when it comes to numbers - dart2js correctly recognized an int in a map value but dart2wasm treated it as a double. Obviously was able to work around by parsing with num, but it was an unexpected surprise.

srujzs commented 5 months ago

@curt-weber I'm assuming this is the same issue as https://github.com/dart-lang/sdk/issues/55203? If so, then yes, it's still a bug we need to fix to get consistency. :)

cmkweber commented 5 months ago

Yep - good find, same issue as the commenter even - wrapper around indexeddb.

srujzs commented 3 months ago

One more discrepancy here that came up in https://github.com/flutter/flutter/issues/153742:

When compiling to JS, we pass through unrelated JS objects if they don't need a conversion in dartify.
When compiling to Wasm, we wrap all such JS objects if they don't need a conversion in dartify.

The idea is to return some interoperable value that users can use, but may lead to issues like https://github.com/flutter/flutter/issues/153742#issuecomment-2299838014 where casts may succeed in one compiler but fail in the other. invalid_runtime_check_with_js_interop_types detects erroneous casts, but the result of dartify is general enough that it doesn't help here.

Another documentation request is to clarify that keys in a Map are converted to JS strings implicitly unless they are Symbols.

dart-lang / sdk

[web/interop] New JS interop extensions that provide `jsify()` / `dartify()` #55222