[web] Remove old futureToPromise. #55847

ditman · 2024-10-12T03:26:02Z

This PR removes the old futureToPromise implementation (which was barely used), and replaces it by a more standard js_interop approach.

Fixes flutter/flutter#155732

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ditman · 2024-10-12T03:26:35Z

lib/web_ui/lib/src/engine/js_interop/js_loader.dart

 abstract class FlutterAppRunner {
  factory FlutterAppRunner({required RunAppFn runApp,}) => FlutterAppRunner._(
-    runApp: (([RunAppFnParameters? args]) => futureToPromise(runApp(args) as Future<JSObject>)).toJS
+    runApp: (([RunAppFnParameters? args]) => (runApp(args) as Future<JSObject>).toPromise).toJS


If I remove the as cast, toPromise stops working (?)

Right, it looks like runApp statically returns a FlutterApp which isn't a JS type since it isn't an interop extension type:

engine/lib/web_ui/lib/src/engine/js_interop/js_app.dart

Line 61 in 01e247a

@JS()

, and therefore your extension might not resolve.

FlutterApp isn't a JS type since it isn't an interop extension type

This makes sense to me. Next steps would be to start migrating this whole js_interop directory to the proper extension-type based interop types.

(That's another Yak, though :P)

lib/web_ui/test/engine/window_test.dart

ditman · 2024-10-12T03:28:26Z

lib/web_ui/lib/src/engine/js_interop/js_promise.dart

+/// This is the same as package:js_interop's FutureToPromise (.toJS), but with
+/// a more descriptive error message.
+///
+/// TODO(srujzs/dit): Move to js_interop's .toJS.


We need to revisit the JS Interop's JSPromise error reporting. The current version boxes hides the message and stack trace in a non-standard place, and makes the reporting in the JS console bad (I'll probably create an issue in dart-lang)

The stack property is somewhat recognized fwiw: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack, but it may be better to populate the error message directly with a stack trace for readability. Hopefully we can come up with something here without a breaking change!

ditman · 2024-10-12T03:30:28Z

/cc @srujzs this is what I ended up with from our talk today!

srujzs

Thanks for digging further, David!

srujzs · 2024-10-14T16:52:21Z

lib/web_ui/lib/src/engine/js_interop/js_promise.dart

+/// This is the same as package:js_interop's FutureToPromise (.toJS), but with
+/// a more descriptive error message.
+///
+/// TODO(srujzs/dit): Move to js_interop's .toJS.


The stack property is somewhat recognized fwiw: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack, but it may be better to populate the error message directly with a stack trace for readability. Hopefully we can come up with something here without a breaking change!

srujzs · 2024-10-14T16:57:28Z

lib/web_ui/lib/src/engine/js_interop/js_loader.dart

 abstract class FlutterAppRunner {
  factory FlutterAppRunner({required RunAppFn runApp,}) => FlutterAppRunner._(
-    runApp: (([RunAppFnParameters? args]) => futureToPromise(runApp(args) as Future<JSObject>)).toJS
+    runApp: (([RunAppFnParameters? args]) => (runApp(args) as Future<JSObject>).toPromise).toJS


Right, it looks like runApp statically returns a FlutterApp which isn't a JS type since it isn't an interop extension type:

engine/lib/web_ui/lib/src/engine/js_interop/js_app.dart

Line 61 in 01e247a

@JS()

, and therefore your extension might not resolve.

srujzs · 2024-10-14T17:12:36Z

lib/web_ui/lib/src/engine/js_interop/js_promise.dart

+  /// A [JSPromise] that either resolves with the result of the completed
+  /// [Future] or rejects with an object that contains its error.
+  JSPromise<T> get toPromise {
+    return JSPromise<T>((JSFunction resolve, JSFunction reject) {


Hmm, the only functional difference between this and toJS I see is that the then value here doesn't return anything in either the onValue or onError functions, but it would be strange if that makes a difference. Are you still seeing the same issues with toJS? Would the sane issue manifest with toPromise if we returned value and wrapper in the onValue and onError functions here?

I think the problem I was seeing was the .toJS not being correctly applied because of the type of the JS Object that was being wrapped in the promise.

The only meaningful difference this has is attempting to compose a string that contains the Dart error text to create the JS Error object when rejecting the promise.

eyebrowsoffire

LGTM!

ditman · 2024-10-21T23:50:55Z

(Updating to apply the auto-submit label after a fresh run of tests)

auto-submit · 2024-10-22T00:26:18Z

auto label is removed for flutter/engine/55847, due to - The status or check suite Mac mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

ditman · 2024-10-22T22:18:30Z

Try again my dear bots!

flutter/engine@50f6d98...ec6e28a 2024-10-23 jonahwilliams@google.com [display_list] grow display list backing store by power of two. (flutter/engine#56004) 2024-10-23 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from xPbin_33tT-_omeQT... to NxrFCTty8wV4-6Cpl... (flutter/engine#56056) 2024-10-23 skia-flutter-autoroll@skia.org Roll Dart SDK from 20470aaa17be to 2a4b728f6a03 (3 revisions) (flutter/engine#56055) 2024-10-23 robert.ancell@canonical.com Move get_keyboard_state from FlKeyboardViewDelegate to FlKeyboardManager. (flutter/engine#56021) 2024-10-23 robert.ancell@canonical.com Move send_key_event from FlKeyboardViewDelegate to FlKeyboardManager. (flutter/engine#56020) 2024-10-23 bdero@google.com [Flutter GPU] Fix assert failure in createDeviceBufferWithCopy. (flutter/engine#56039) 2024-10-22 skia-flutter-autoroll@skia.org Roll Dart SDK from 8de85635d731 to 20470aaa17be (4 revisions) (flutter/engine#56038) 2024-10-22 ditman@gmail.com [web] Remove old futureToPromise. (flutter/engine#55847) 2024-10-22 737941+loic-sharma@users.noreply.github.com [dart:ui] Improve Clip's docs (flutter/engine#55978) 2024-10-22 jonahwilliams@google.com [Impeller] flush thread local resources during toImage/toImageSync. (flutter/engine#56037) 2024-10-22 kpsroka@users.noreply.github.com [canvaskit] Makes access to CkSurface null-safer (flutter/engine#54895) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from xPbin_33tT-_ to NxrFCTty8wV4 If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC codefu@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…57453) flutter/engine@50f6d98...ec6e28a 2024-10-23 jonahwilliams@google.com [display_list] grow display list backing store by power of two. (flutter/engine#56004) 2024-10-23 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from xPbin_33tT-_omeQT... to NxrFCTty8wV4-6Cpl... (flutter/engine#56056) 2024-10-23 skia-flutter-autoroll@skia.org Roll Dart SDK from 20470aaa17be to 2a4b728f6a03 (3 revisions) (flutter/engine#56055) 2024-10-23 robert.ancell@canonical.com Move get_keyboard_state from FlKeyboardViewDelegate to FlKeyboardManager. (flutter/engine#56021) 2024-10-23 robert.ancell@canonical.com Move send_key_event from FlKeyboardViewDelegate to FlKeyboardManager. (flutter/engine#56020) 2024-10-23 bdero@google.com [Flutter GPU] Fix assert failure in createDeviceBufferWithCopy. (flutter/engine#56039) 2024-10-22 skia-flutter-autoroll@skia.org Roll Dart SDK from 8de85635d731 to 20470aaa17be (4 revisions) (flutter/engine#56038) 2024-10-22 ditman@gmail.com [web] Remove old futureToPromise. (flutter/engine#55847) 2024-10-22 737941+loic-sharma@users.noreply.github.com [dart:ui] Improve Clip's docs (flutter/engine#55978) 2024-10-22 jonahwilliams@google.com [Impeller] flush thread local resources during toImage/toImageSync. (flutter/engine#56037) 2024-10-22 kpsroka@users.noreply.github.com [canvaskit] Makes access to CkSurface null-safer (flutter/engine#54895) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from xPbin_33tT-_ to NxrFCTty8wV4 If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC codefu@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR removes the old `futureToPromise` implementation (which was barely used), and replaces it by a more standard js_interop approach. Fixes flutter#155732 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Remove old futureToPromise function.

e1e0a2f

github-actions bot added the platform-web Code specifically for the web engine label Oct 12, 2024

ditman commented Oct 12, 2024

View reviewed changes

lib/web_ui/test/engine/window_test.dart Show resolved Hide resolved

ditman commented Oct 12, 2024

View reviewed changes

ditman requested a review from srujzs October 12, 2024 03:30

srujzs approved these changes Oct 14, 2024

View reviewed changes

ditman mentioned this pull request Oct 14, 2024

[web] Improve error messages in Future.toJS dart-lang/sdk#56898

Open

Make TODO conform to style guide

2b39b78

ditman requested review from eyebrowsoffire and mdebbar October 14, 2024 23:43

ditman marked this pull request as ready for review October 15, 2024 18:51

eyebrowsoffire approved these changes Oct 21, 2024

View reviewed changes

Merge branch 'main' into web-more-prominent-errors

337e55f

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 22, 2024

Merge branch 'main' into web-more-prominent-errors

494a220

ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 22, 2024

auto-submit bot merged commit 5d3a474 into flutter:main Oct 22, 2024

engine-flutter-autoroll mentioned this pull request Oct 23, 2024

Roll Flutter Engine from 50f6d98cb8be to d32cfc393662 (10 revisions) flutter/flutter#157448

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2024

5d3a474 [web] Remove old futureToPromise. (flutter/engine#55847)

3df7ade

engine-flutter-autoroll mentioned this pull request Oct 23, 2024

Roll Flutter Engine from 50f6d98cb8be to ec6e28ac8b49 (11 revisions) flutter/flutter#157453

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2024

5d3a474 [web] Remove old futureToPromise. (flutter/engine#55847)

33d6237

[web] Remove old futureToPromise. #55847

[web] Remove old futureToPromise. #55847

Uh oh!

Conversation

ditman commented Oct 12, 2024

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman commented Oct 12, 2024

Uh oh!

srujzs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyebrowsoffire left a comment

Choose a reason for hiding this comment

Uh oh!

ditman commented Oct 21, 2024

Uh oh!

auto-submit bot commented Oct 22, 2024

Uh oh!

ditman commented Oct 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ditman Oct 12, 2024 •

edited

Loading