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

Remove some of our hacks around JSPromise now that we have better APIs. #45591

Merged
merged 3 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 11 additions & 18 deletions lib/web_ui/lib/src/engine/app_bootstrap.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:js/js_util.dart' show allowInterop;
import 'dart:js_interop';

import 'configuration.dart';
import 'js_interop/js_loader.dart';
Expand Down Expand Up @@ -40,33 +40,26 @@ class AppBootstrap {
// This is a convenience method that lets the programmer call "autoStart"
// from JavaScript immediately after the main.dart.js has loaded.
// Returns a promise that resolves to the Flutter app that was started.
autoStart: allowInterop(() => futureToPromise(() async {
autoStart: (() => futureToPromise(() async {
await autoStart();
// Return the App that was just started
return _prepareFlutterApp();
}())),
return _prepareFlutterApp() as JSAny;
srujzs marked this conversation as resolved.
Show resolved Hide resolved
}())).toJS,
// Calls [_initEngine], and returns a JS Promise that resolves to an
// app runner object.
initializeEngine: allowInterop(([JsFlutterConfiguration? configuration]) => futureToPromise(() async {
initializeEngine: (([JsFlutterConfiguration? configuration]) => futureToPromise(() async {
await _initializeEngine(configuration);
return _prepareAppRunner();
}()))
return _prepareAppRunner() as JSAny;
}())).toJS
);
}

/// Creates an appRunner that runs our encapsulated runApp function.
FlutterAppRunner _prepareAppRunner() {
return FlutterAppRunner(runApp: allowInterop(([RunAppFnParameters? params]) {
// `params` coming from JS may be used to configure the run app method.
return Promise<FlutterApp>(allowInterop((
PromiseResolver<FlutterApp> resolve,
PromiseRejecter _,
) async {
await _runApp();
// Return the App that was just started
resolve.resolve(_prepareFlutterApp());
}));
}));
return FlutterAppRunner(runApp: (([RunAppFnParameters? params]) => futureToPromise(() async {
await _runApp();
return _prepareFlutterApp() as JSAny;
}())).toJS);
}

/// Represents the App that was just started, and its JS API.
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/canvaskit/image_web_codecs.dart
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ Future<ByteBuffer> readVideoFramePixelsUnmodified(VideoFrame videoFrame) async {
// In dart2wasm, Uint8List is not the same as a JS Uint8Array. So we
// explicitly construct the JS object here.
final JSUint8Array destination = createUint8ArrayFromLength(size);
final JsPromise copyPromise = videoFrame.copyTo(destination);
final JSPromise copyPromise = videoFrame.copyTo(destination);
await promiseToFuture<void>(copyPromise);

// In dart2wasm, `toDart` incurs a copy here. On JS backends, this is a
Expand Down
23 changes: 3 additions & 20 deletions lib/web_ui/lib/src/engine/js_interop/js_loader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ import 'dart:js_interop';

import 'package:js/js_util.dart' as js_util;

import '../configuration.dart';
import 'js_promise.dart';

@JS()
@staticInterop
class FlutterJS {}
Expand Down Expand Up @@ -45,22 +42,11 @@ extension FlutterLoaderExtension on FlutterLoader {
@staticInterop
abstract class FlutterEngineInitializer{
external factory FlutterEngineInitializer({
required InitializeEngineFn initializeEngine,
required ImmediateRunAppFn autoStart,
required JSFunction initializeEngine,
required JSFunction autoStart,
});
}

/// Typedef for the function that initializes the flutter engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's up to you if you want to make this typed a bit better by having a non-external function that takes in the typedefs, does the toJS conversions, and then forwards them to a (private) external factory function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess maybe it's better to keep a little bit of type safety here.

///
/// [JsFlutterConfiguration] comes from `../configuration.dart`. It is the same
/// object that can be used to configure flutter "inline", through the
/// (to be deprecated) `window.flutterConfiguration` object.
typedef InitializeEngineFn = Promise<FlutterAppRunner> Function([JsFlutterConfiguration?]);

/// Typedef for the `autoStart` function that can be called straight from an engine initializer instance.
/// (Similar to [RunAppFn], but taking no specific "runApp" parameters).
typedef ImmediateRunAppFn = Promise<FlutterApp> Function();

// FlutterAppRunner

/// A class that exposes a function that runs the Flutter app,
Expand All @@ -71,7 +57,7 @@ typedef ImmediateRunAppFn = Promise<FlutterApp> Function();
abstract class FlutterAppRunner {
/// Runs a flutter app
external factory FlutterAppRunner({
required RunAppFn runApp, // Returns an App
required JSFunction runApp, // Returns an App
});
}

Expand All @@ -83,9 +69,6 @@ abstract class FlutterAppRunner {
abstract class RunAppFnParameters {
}

/// Typedef for the function that runs the flutter app main entrypoint.
typedef RunAppFn = Promise<FlutterApp> Function([RunAppFnParameters?]);

// FlutterApp

/// A class that exposes the public API of a running Flutter Web App running.
Expand Down
41 changes: 14 additions & 27 deletions lib/web_ui/lib/src/engine/js_interop/js_promise.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,40 +11,27 @@ import 'package:js/js_util.dart' as js_util;

import '../util.dart';

@JS()
@staticInterop
class PromiseResolver<T extends Object?> {}

extension PromiseResolverExtension<T extends Object?> on PromiseResolver<T> {
void resolve(T result) => js_util.callMethod(this, 'call', <Object>[this, if (result != null) result]);
}

@JS()
@staticInterop
class PromiseRejecter {}

extension PromiseRejecterExtension on PromiseRejecter {
void reject(Object? error) => js_util.callMethod(this, 'call', <Object>[this, if (error != null) error]);
extension CallExtension on JSFunction {
external void call(JSAny? this_, JSAny? object);
}

/// Type-safe JS Promises
@JS('Promise')
@staticInterop
abstract class Promise<T extends Object?> {
/// A constructor for a JS promise
external factory Promise(PromiseExecutor<T> executor);
}
external JSAny get _promiseConstructor;

JSPromise createPromise(JSFunction executor) =>
srujzs marked this conversation as resolved.
Show resolved Hide resolved
js_util.callConstructor(
_promiseConstructor,
<Object>[executor],
);

/// The type of function that is used to create a Promise<T>
typedef PromiseExecutor<T extends Object?> = void Function(PromiseResolver<T> resolve, PromiseRejecter reject);

Promise<T> futureToPromise<T extends Object>(Future<T> future) {
return Promise<T>(js_util.allowInterop((PromiseResolver<T> resolver, PromiseRejecter rejecter) {
JSPromise futureToPromise<T extends JSAny>(Future<T> future) {
return createPromise((JSFunction resolver, JSFunction rejecter) {
future.then(
(T value) => resolver.resolve(value),
(T value) => resolver.call(null, value),
onError: (Object? error) {
printWarning('Rejecting promise with error: $error');
rejecter.reject(error);
rejecter.call(null, null);
});
}));
}.toJS);
}
22 changes: 4 additions & 18 deletions lib/web_ui/lib/src/engine/safe_browser_api.dart
Original file line number Diff line number Diff line change
Expand Up @@ -196,20 +196,6 @@ bool get _defaultBrowserSupportsImageDecoder =>
// enable it explicitly.
bool get _isBrowserImageDecoderStable => browserEngine == BrowserEngine.blink;

/// The signature of the function passed to the constructor of JavaScript `Promise`.
typedef JsPromiseCallback = void Function(void Function(Object? value) resolve, void Function(Object? error) reject);

/// Corresponds to JavaScript's `Promise`.
///
/// This type doesn't need any members. Instead, it should be first converted
/// to Dart's [Future] using [promiseToFuture] then interacted with through the
/// [Future] API.
@JS('window.Promise')
@staticInterop
class JsPromise {
external factory JsPromise(JsPromiseCallback callback);
}

/// Corresponds to the browser's `ImageDecoder` type.
///
/// See also:
Expand All @@ -228,7 +214,7 @@ extension ImageDecoderExtension on ImageDecoder {
external JSBoolean get _complete;
bool get complete => _complete.toDart;

external JsPromise decode(DecodeOptions options);
external JSPromise decode(DecodeOptions options);
external JSVoid close();
}

Expand Down Expand Up @@ -302,8 +288,8 @@ extension VideoFrameExtension on VideoFrame {
double allocationSize() => _allocationSize().toDartDouble;

@JS('copyTo')
external JsPromise _copyTo(JSAny destination);
JsPromise copyTo(Object destination) => _copyTo(destination.toJSAnyShallow);
external JSPromise _copyTo(JSAny destination);
JSPromise copyTo(Object destination) => _copyTo(destination.toJSAnyShallow);

@JS('format')
external JSString? get _format;
Expand Down Expand Up @@ -344,7 +330,7 @@ extension VideoFrameExtension on VideoFrame {
class ImageTrackList {}

extension ImageTrackListExtension on ImageTrackList {
external JsPromise get ready;
external JSPromise get ready;
external ImageTrack? get selectedTrack;
}

Expand Down
20 changes: 10 additions & 10 deletions lib/web_ui/test/engine/window_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:js_interop';
import 'dart:js_util' as js_util;
import 'dart:typed_data';

Expand Down Expand Up @@ -331,19 +332,18 @@ Future<void> testMain() async {
// The `orientation` property cannot be overridden, so this test overrides the entire `screen`.
js_util.setProperty(domWindow, 'screen', js_util.jsify(<Object?, Object?>{
'orientation': <Object?, Object?>{
'lock': allowInterop((String lockType) {
'lock': (String lockType) {
lockCalls.add(lockType);
return Promise<Object?>(allowInterop((PromiseResolver<Object?> resolve, PromiseRejecter reject) {
if (!simulateError) {
resolve.resolve(null);
} else {
reject.reject('Simulating error');
return futureToPromise(() async {
if (simulateError) {
throw Error();
}
}));
}),
'unlock': allowInterop(() {
return 0.toJS;
}());
}.toJS,
'unlock': () {
unlockCount += 1;
}),
}.toJS,
},
}));

Expand Down