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

Add docs for ExternalDartReference #5668

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Mar 26, 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.

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.
@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Mar 26, 2024

Visit the preview URL for this PR (updated for commit 2409ab4):

https://dart-dev--pr5668-addexternaldartreference-45iczn14.web.app

Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only some suggestions, but LGTM even as is

@@ -38,6 +38,15 @@ JS types form a natural type hierarchy:

You can find the definition of each type in the [`dart:js_interop` API docs].

From Dart 3.4 onwards, there also exists one type in `dart:js_interop` that can
be used but is *not* a JS type called `ExternalDartReference`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"be used" here may be missing context (e.g. used for what purpose?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Dart `Object`s through JavaScript. However, `JSBoxedDartObject` wraps the opaque
reference in a JavaScript object, while `ExternalDartReference` is the reference
itself and therefore is not a JS type. Use `ExternalDartReference` where
performance is needed and `JSBoxedDartObject` where a JS type is needed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While correct, I'm not sure if we have an intuition for when "a JS type is needed".

Aside frokm being required by a type signature, are there use-cases we can describe where it makes sense to use one or the other? For example, if you just want to pass references opaquely back and forth and using the reference just as an identity, then ExternalDartReference seems like a good fit. If you however want a reference that can also store data and you want to extend it to add data to that reference, then we recommend using JSBoxedDartObject. However, the latter feels like a strange and uncommon scenario. Are there other scenarios where we would be inclined to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think needed here is entirely specific to the type signature. It may be "needed" in order to pass the Dart object to an API that accepts a JSAny, or place within a JSArray. I don't see much reason for anyone to actually interact with a JSBoxedDartObject. I added a comment to illustrate what "a JS type is needed" might mean.

@@ -73,7 +82,7 @@ Generally, the conversion table looks like the following:
<div class="table-wrapper" markdown="1">

| JS type | Dart type |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional, maybe something to consider as a separate follow up change)

I'm curious about generalizing this table to include ExternalDartReference. Maybe if we were to rename "JS type" to something more general, we could?

At first I was going to suggest "Interop type", but I believe we use JS-type and interop-type interchangingly.

Copy link
Contributor

@MaryaBelanger MaryaBelanger Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we use JS-type and interop-type interchangingly.

We actually don't use them interchangeably. I think JS types are interop types, but not all interop types are JS types. From the Usage page:

Interop types are either a "JS type" provided by Dart or an extension type wrapping an interop type.

Edit: (Forgot to conclude) So if ExternalDartReference fits that more general definition of interop types, then I think we could change the column name to Interop types. That would allow an other conversions of note to be included too (if there are any)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - JS types are a subset of interop types.

I thought about initially putting ExternalDartReference here, but the name gave me pause. Interop types works and lets it fit in nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I changed this to dart:js_interop type instead of just Interop type so that it avoids contradicting the above definition for ExternalDartReference.

Copy link
Contributor

@MaryaBelanger MaryaBelanger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for incorporating my suggestions! One more thing about the links but otherwise this looks great

src/content/interop/js-interop/js-types.md Outdated Show resolved Hide resolved
@MaryaBelanger MaryaBelanger merged commit a7e58cc into dart-lang:main Apr 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants