Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
9 tasks
mkustermann opened this issue Mar 18, 2024 · 8 comments
Open
9 tasks
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop

Comments

@mkustermann
Copy link
Member

mkustermann commented Mar 18, 2024

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. [bug wasm/js] jsify/dartify issue converting a simple int #55203)
  • Should we offer an option to throw if numbers loose precision when crossing boundary

/cc @srujzs @sigmundch

@mkustermann mkustermann added the web-js-interop Issues that impact all js interop label Mar 18, 2024
@mit-mit mit-mit added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Mar 18, 2024
@srujzs
Copy link
Contributor

srujzs commented Mar 18, 2024

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
Copy link
Member Author

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

@srujzs
Copy link
Contributor

srujzs commented Mar 19, 2024

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
Copy link
Contributor

srujzs commented Apr 19, 2024

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

if (_noJsifyRequired(object)) {

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

@curt-weber
Copy link

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
Copy link
Contributor

srujzs commented May 31, 2024

@curt-weber I'm assuming this is the same issue as #55203? If so, then yes, it's still a bug we need to fix to get consistency. :)

@cmkweber
Copy link

cmkweber commented Jun 1, 2024

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

@srujzs
Copy link
Contributor

srujzs commented Aug 21, 2024

One more discrepancy here that came up in flutter/flutter#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 flutter/flutter#153742 (comment) 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop
Projects
Status: No status
Development

No branches or pull requests

5 participants