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

Expose JSUnsafeDartObject in dart:js_interop_unsafe #55187

Closed
srujzs opened this issue Mar 13, 2024 · 6 comments
Closed

Expose JSUnsafeDartObject in dart:js_interop_unsafe #55187

srujzs opened this issue Mar 13, 2024 · 6 comments
Assignees
Labels
area-dart2wasm Issues for the dart2wasm compiler. 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

@srujzs
Copy link
Contributor

srujzs commented Mar 13, 2024

Externalizing some internal discussion on this topic already:

Currently, the way to pass Dart objects to JavaScript involves JSBoxedDartObject. Its conversion method, toJSBox does:

  • On the JS backends, we create a JS object and then set a Dart runtime-specific symbol whose value is the Dart object.
  • On dart2wasm, we do something very similar except the Dart object is converted to an externref first.

While this offers a consistent interface on the JavaScript side and avoids mixing Dart objects between runtimes, it does come with downsides:

  1. It's slow, especially if passing Dart objects to JS is a common operation.
  2. It creates a new box everytime the Dart object is converted, requiring users to be careful about identity.
  3. Users aren't meant to actually access the Dart value in the box.

Instead, we can provide a better type here which doesn't provide any consistency or runtime safety, but addresses the above downsides. As it's not a JS value, it shouldn't be a subtype of JSAny and our error checker should allowlist this value to flow into JS.

Related issues:

#55183
#55114
#54925

@srujzs srujzs added web-js-interop Issues that impact all js interop area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. area-dart2wasm Issues for the dart2wasm compiler. labels Mar 13, 2024
@mkustermann
Copy link
Member

I think there's nothing unsafe about it - it's simply a lower-level primitive.

This should be easily accessible, it should be the default way of doing things. Putting it into dart:js_interop_unsafe would indicate to users that there's big dangers in using this - I'd strongly advise against this:

  • the default of crossing boundary to js should be fast, not slow
  • common use case is to have only one dart2wasm compiled app running in the same browser context
  • most common usecase is to pass it out to JS and JS passes it back to the very same app (just like an opaque handle - exactly the use case in flutter)

Instead I'd suggest to add an extension to dart:js_interop with an appropriate named getter

extension on Object {
  JSExternalizedObject get externalize;
  // or
  JSWrapper get toJSWrapper;
  // or
  JSCompanion get toJSCompanion;
  // or
  JS... get <some-better-name>;
}

/cc @sigmundch

@sigmundch
Copy link
Member

Thanks for the additional input @mkustermann!

I'm open to moving this to be the default in dart:js_interop.

I would only want to go through the exercise of recalling the reasons we initially suggested to put this in js_interop_unsafe to make sure we are not missing something important.

When we first proposed it, we had two safety goals in mind for the Dart/JS interop design: type soundness (which prior interop didn't have) and enforced separation of Dart and JS type hierarchies. We dropped the second goal when we discovered that it would have been too expensive to enforce such separation. In fact, it would have made type soundness even more expensive to achieve. (In case it helps, I also provided you with some additional references offline for context.)

It all boiled down to what was our choice for what JSAny is refied as. Today it gets erased to Object on JS backends, but for a long time we had in mind to use a different top type. If we had picked a different reification, passing objects unboxed would have introduced the possibility of breaking type unsoundness. True, this was mainly in the context of multi-app scenarios which are rare for dart2wasm, but very common on JS backends today.

I believe that was the most likely reason we deemed it as unsafe, and I can't recall at the moment if there was something else. Given that since then we have made the choice of reify JsAny as Object, I think it's valid to change our approach here.

@srujzs - curious to hear your thoughts here too.

@srujzs
Copy link
Contributor Author

srujzs commented Mar 14, 2024

I also think it's possible to put this in dart:js_interop.

Beyond 1) soundness and 2) separation, I also was concerned about 3) consistency guarantees.

I think soundness is trivially possible to achieve here, and this API wouldn't affect that.

We were worried about separation if we wanted to make JSAny eventually reify to Interceptor. For other reasons, like how DDC reifies types or how certain List conversions would become harder, we ended up not going down this route and stayed with Object. However, as I just mentioned internally, if we don't make this a subtype of JSAny, we still have that option in the future. I don't see much benefit in making this a subtype of JSAny, and it better communicates the reality that this is still a Dart value and not a JS value.

I think consistency of the externalized value is impossible across all web backends, hence I was thinking of declaring it as "unsafe". JSBoxedDartObject offers this consistency on the other hand. Granted, we have consistency issues elsewhere, like with List conversions or with how we represent JS types, but we didn't have a type that had a different representation in JavaScript until this. Maybe this is not an issue, and any doc comment should certainly communicate this reality, so I'm okay with adding this to dart:js_interop as well.

An alternative naming scheme might be something like JSRawDartObject and toJSRaw to contrast this with JSBoxedDartObject.

@srujzs srujzs self-assigned this Mar 14, 2024
@sigmundch
Copy link
Member

Great! Thanks @srujzs!

Re naming. Another idea is JSUnboxedDartObject

As for the getter, I wonder if we should repurpose .toJS to be this one, and add .toJSBox for the old semantics. It could potentially be breaking in some rare cases, but if we are really trying to encourage the fast-path by default, maybe we should also give it the common name?

@srujzs
Copy link
Contributor Author

srujzs commented Mar 14, 2024

There is no toJS for Objects today, only toJSBox, and that was intentional. I'd like to avoid adding toJS for this purpose for several reasons:

  • Since it would be available on all Objects, this may lead users to believe that all Dart Objects can be converted to a JS type, which isn't true.
  • Since toJS is the name of most conversion methods, users may think that this toJS is doing something like jsify, which isn't true. Extension member resolution chooses the most specific subtype, so name conflicts aren't really an issue.

I'm also open to not naming this type with a JS prefix at all considering this isn't a JS type really. It's not a subtype of JSAny nor is it a JS value. Maybe something like ExternalizedInteropObject? I'll have to brainstorm more. :)

@sigmundch
Copy link
Member

Great points! Happy to brainstorm offline. Once we land on something, we should follow up with an update on our docs as well.

srujzs added a commit to srujzs/site-www that referenced this issue Mar 26, 2024
This type exists in 3.4 onwards but isn't a JS type. See
dart-lang/sdk#55187 for more
details on what the type is.
MaryaBelanger pushed a commit to dart-lang/site-www that referenced this issue Apr 2, 2024
This type was added as part of
dart-lang/sdk#55187 to dart:js_interop for a
faster alternative to JSBoxedDartObject, so we should add some
documentation on it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler. 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: Done
Development

No branches or pull requests

3 participants