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] Migrate DOM shim to JS types. #40310

Merged
merged 1 commit into from Mar 17, 2023
Merged

Conversation

joshualitt
Copy link
Contributor

This CL is the first in a small set of CLs to migrate the DOM shim to JS types.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 15, 2023
@joshualitt joshualitt force-pushed the js-types branch 2 times, most recently from 9d8d713 to c4e3526 Compare March 15, 2023 17:21
@joshualitt joshualitt marked this pull request as ready for review March 15, 2023 20:11
@joshualitt
Copy link
Contributor Author

@ditman @eyebrowsoffire @mdebbar , this is basically ready for review. There may be some bugs and back and forth while I do some framework testing, but this CL is large, and we want to land it as quickly as possible so we can unblock other efforts, so it makes sense to start the review ASAP.

These will be the patterns for JS interop we will use until we have inline classes, which will be later this year. That migration will be much smaller than this one as it just involves changing how the classes are defined.

Some things to note:

  1. Some places Wasm needs to convert objects, whereas JS backends don't. This logic is hidden behind the 'deep' / 'shallow' conversions in the dom shim. Sorry about the naming scheme. I'm open to alternatives, but I'm not sure there are any good names in this space as the underlying concept we are trying to name(i.e. conversions required on JS backends but not on Wasm backends) is quite vague.
  2. Almost all of this migration logic was hidden safely behind the shim. The exceptions are functions. With JSFunction we will no longer have the problem where people forget to wrap Dart functions when the flow to JS. However, as you can see in this CL there is some subtlety here. I left the event listeners alone for now, and tried to touch only those functions which get 'thrown over the wall'.
  3. I tried to start getting rid of uses of js_util but I didn't push this very hard in this CL, with one notable exception. We were using js_util to handle optional arguments. I changed these usages to trampolines. However, we've fixed optional arguments to JS functions. Now they have JS semantics(i.e. not passed is undefined), which is quite nice. Unfortunately because this logic doesn't work for tearoffs, I figured we should discuss that change and possibly do it in a follow on if we wish. I will push further on getting rid of as many usages of js_util as possible in the future, though some usages may be unavoidable.
  4. Please let me know if you'd like more comments, or have any questions / concerns.

@joshualitt
Copy link
Contributor Author

@srujzs FYI. Please feel free to leave comments.

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Overall, the change LGTM if it LGT @eyebrowsoffire. I just have a few questions to understand things better.

lib/web_ui/lib/src/engine/initialization.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/platform_dispatcher.dart Outdated Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

This is great! If everybody else's happy, and tests pass; I'm happy!

Also I've read in a comment to Mouad that we have rules that are (going to be) automatically enforceable when compiling libraries, so hit it!

lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/initialization.dart Show resolved Hide resolved
lib/web_ui/lib/src/engine/platform_dispatcher.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

Looks good with a minor nitpick. I also wanted to share a few non-blocking thoughts on how this is shaping up.

  1. One thing that bothers me a little bit is JSNumber.toDart, which secretly means "double". I think a toDart function is perfectly fine for things where their translation is immediately obvious, like JSString -> String. But for things that are actually ambiguous like JSNumber, I sort of wish it had toInt and toDouble but no toDart. I understand that toDart is actually on Object (I think?) so maybe this is not that tractable, but I thought I'd share my general feeling about this.
  2. Definitely not necessary for this PR or anything like that, but I actually feel like a lot of the toDart and toJS calls should actually go to the call sites, not in dom.dart. dom.dart is more or less a direct representation of the browser API itself, and as such, if you're using the APIs exposed in here, you are very much aware that you are interacting with JavaScript. Therefore, I believe it is on the layer above to decide if and how to do that marshaling.

There are many situations in which we might be taking a JSString directly from a browser API and passing it back into another browser API, which doesn't necessarily need a translation. With this setup, we are always doing conversions back and forth even if we don't need it. This combined with the fact that the profiles of wasm startup are heavily bogged down in JS<->Dart string conversions, I think we should really start thinking about doing another pass at some point that makes all the APIs in dom.dart deal with JS objects purely, and modify the call sites. That being said, this PR is probably the simpler and more iterative way to do it, so I think that is a bridge to cross another day.

lib/web_ui/lib/src/engine/dom.dart Show resolved Hide resolved
@joshualitt
Copy link
Contributor Author

  1. One thing that bothers me a little bit is JSNumber.toDart, which secretly means "double". I think a toDart function is perfectly fine for things where their translation is immediately obvious, like JSString -> String. But for things that are actually ambiguous like JSNumber, I sort of wish it had toInt and toDouble but no toDart. I understand that toDart is actually on Object (I think?) so maybe this is not that tractable, but I thought I'd share my general feeling about this.

This is an interesting idea. I definitely see both sides, and there are a lot of points in the design space. Let's roll out strict- mode for JS types first, and then we can revisit how exactly we want to handle explicit conversions.

  1. Definitely not necessary for this PR or anything like that, but I actually feel like a lot of the toDart and toJS calls should actually go to the call sites, not in dom.dart. dom.dart is more or less a direct representation of the browser API itself, and as such, if you're using the APIs exposed in here, you are very much aware that you are interacting with JavaScript. Therefore, I believe it is on the layer above to decide if and how to do that marshaling.

Huge +1 to this, let's do this work as a follow on. It will be a nice cleanup that will make it crystal clear to engine developers where the exact boundary between Dart and JS is. I also agree with your point on efficiency. There are probably many places where these conversions are not necessary, even in a local sense, let alone if we decide we want to hold on to JS types in maps and so forth instead of their Dart counterparts. I added a comment.

@joshualitt joshualitt merged commit 59e21ff into flutter:main Mar 17, 2023
34 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 17, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 17, 2023
Roll Flutter Engine from 476985ae3014 to 59e21ffbf9b8 (1 revision)
sourcegraph-bot pushed a commit to sgtest/megarepo that referenced this pull request Mar 17, 2023
…122895)

Commit: 038eea6312b4f868488d08807d788f4a6133e923
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-web Code specifically for the web engine
Projects
None yet
5 participants