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.

Sign up for GitHub

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

Jump to bottom

Fix JS interop signatures to use only JS types. #45668

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Fix JS interop signatures to use only JS types. #45668

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92c9e0d
Fix JS interop signatures to use only JS types.
eyebrowsoffire Sep 11, 2023
404c945
Make runApp have optional parameters.
eyebrowsoffire Sep 11, 2023
0f2a316
Pass intitialize engine args along.
eyebrowsoffire Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 8 additions & 18 deletions lib/web_ui/lib/src/engine/app_bootstrap.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,8 @@
// 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 'configuration.dart';
import 'js_interop/js_loader.dart';
import 'js_interop/js_promise.dart';

/// The type of a function that initializes an engine (in Dart).
typedef InitEngineFn = Future<void> Function([JsFlutterConfiguration? params]);
Expand Down Expand Up @@ -40,33 +37,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: () async {
await autoStart();
// Return the App that was just started
return _prepareFlutterApp();
}())),
},
// Calls [_initEngine], and returns a JS Promise that resolves to an
// app runner object.
initializeEngine: allowInterop(([JsFlutterConfiguration? configuration]) => futureToPromise(() async {
initializeEngine: ([JsFlutterConfiguration? configuration]) async {
await _initializeEngine(configuration);
return _prepareAppRunner();
}()))
}
);
}

/// 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]) async {
await _runApp();
return _prepareFlutterApp();
});
}

/// 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
View file
Open in desktop
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
49 changes: 29 additions & 20 deletions lib/web_ui/lib/src/engine/js_interop/js_loader.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ library js_loader;
import 'dart:js_interop';

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

import '../configuration.dart';
import 'js_promise.dart';
import 'package:ui/src/engine.dart';

@JS()
@staticInterop
Expand All @@ -34,6 +32,17 @@ extension FlutterLoaderExtension on FlutterLoader {
bool get isAutoStart => !js_util.hasProperty(this, 'didCreateEngineInitializer');
}

/// Typedef for the function that initializes the flutter engine.
/// ///
/// [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 = Future<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 = Future<FlutterApp> Function();

// FlutterEngineInitializer

/// An object that allows the user to initialize the Engine of a Flutter App.
Expand All @@ -44,34 +53,34 @@ extension FlutterLoaderExtension on FlutterLoader {
@anonymous
@staticInterop
abstract class FlutterEngineInitializer{
external factory FlutterEngineInitializer({
factory FlutterEngineInitializer({
required InitializeEngineFn initializeEngine,
required ImmediateRunAppFn autoStart,
}) => FlutterEngineInitializer._(
initializeEngine: (() => futureToPromise(initializeEngine())).toJS,
Copy link
Member

Choose a reason for hiding this comment

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

Is this swallowing the parameters passed to the initializeEngine fn from JS?

I suspect this needs to be:

initializeEngine: (([JsFlutterConfiguration?] args) => futureToPromise(initializeEngine(args))).toJS,

(testing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dang, you're right. Let me fix that.

autoStart: (() => futureToPromise(autoStart())).toJS,
);
external factory FlutterEngineInitializer._({
required JSFunction initializeEngine,
required JSFunction autoStart,
});
}

/// Typedef for the function that initializes the flutter engine.
///
/// [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,
/// and returns a promise of a FlutterAppCleaner.
@JS()
@anonymous
@staticInterop
abstract class FlutterAppRunner {
abstract class FlutterAppRunner extends JSObject {
factory FlutterAppRunner({required RunAppFn runApp,}) => FlutterAppRunner._(
runApp: (([RunAppFnParameters? args]) => futureToPromise(runApp(args))).toJS
);

/// Runs a flutter app
external factory FlutterAppRunner({
required RunAppFn runApp, // Returns an App
external factory FlutterAppRunner._({
required JSFunction runApp, // Returns an App
});
}

Expand All @@ -84,15 +93,15 @@ abstract class RunAppFnParameters {
}

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

// FlutterApp

/// A class that exposes the public API of a running Flutter Web App running.
@JS()
@anonymous
@staticInterop
abstract class FlutterApp {
abstract class FlutterApp extends JSObject {
/// Cleans a Flutter app
external factory FlutterApp();
}
41 changes: 14 additions & 27 deletions lib/web_ui/lib/src/engine/js_interop/js_promise.dart
View file
Open in desktop
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) =>
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
View file
Open in desktop
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
22 changes: 11 additions & 11 deletions lib/web_ui/test/canvaskit/canvaskit_api_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1847,8 +1847,8 @@ void _paragraphTests() {
}, skip: isFirefox); // Intended: Headless firefox has no webgl support https://github.com/flutter/flutter/issues/109265

group('getCanvasKitJsFileNames', () {
dynamic oldV8BreakIterator = v8BreakIterator;
dynamic oldIntlSegmenter = intlSegmenter;
JSAny? oldV8BreakIterator = v8BreakIterator;
JSAny? oldIntlSegmenter = intlSegmenter;

setUp(() {
oldV8BreakIterator = v8BreakIterator;
Expand All @@ -1861,8 +1861,8 @@ void _paragraphTests() {
});

test('in Chromium-based browsers', () {
v8BreakIterator = Object(); // Any non-null value.
intlSegmenter = Object(); // Any non-null value.
v8BreakIterator = Object().toJSBox; // Any non-null value.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does toJSBox do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In dart2wasm, all dart objects are wasm structs. There is a wasm opcode to externalize these structs and create an externref, and that externref can be passed to the JS host environment. To JS, that object is a sort of special opaque object with an immutable prototype, and that can be passed back to wasm and internalized again if needed.

In dart2js, I think this actually creates some sort of secondary wrapper around the dart object, but that also might be changing soon to do nothing and just pass the dart object (which is just a JS object) around, and only really changes the static type of the object from the dart language perspective.

In any case, it doesn't matter much in this scenario, just that it creates a non-null, non-falsey JS value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just +1'ing here. The desire was to avoid users attempting to interface with Dart objects on the JS side or accidentally passing it to different runtimes. For the JS backends, this does mean that we need an actual wrapper, which in practice is just an object literal with a runtime-specific symbol to store the object in. This may indeed change based on some internal feedback, but shouldn't affect how this code is being used.

intlSegmenter = Object().toJSBox; // Any non-null value.
browserSupportsImageDecoder = true;

expect(getCanvasKitJsFileNames(CanvasKitVariant.full), <String>['canvaskit.js']);
Expand All @@ -1874,7 +1874,7 @@ void _paragraphTests() {
});

test('in older versions of Chromium-based browsers', () {
v8BreakIterator = Object(); // Any non-null value.
v8BreakIterator = Object().toJSBox; // Any non-null value.
intlSegmenter = null; // Older versions of Chromium didn't have the Intl.Segmenter API.
browserSupportsImageDecoder = true;

Expand All @@ -1884,15 +1884,15 @@ void _paragraphTests() {
});

test('in other browsers', () {
intlSegmenter = Object(); // Any non-null value.
intlSegmenter = Object().toJSBox; // Any non-null value.

v8BreakIterator = null;
browserSupportsImageDecoder = true;
expect(getCanvasKitJsFileNames(CanvasKitVariant.full), <String>['canvaskit.js']);
expect(getCanvasKitJsFileNames(CanvasKitVariant.chromium), <String>['chromium/canvaskit.js']);
expect(getCanvasKitJsFileNames(CanvasKitVariant.auto), <String>['canvaskit.js']);

v8BreakIterator = Object();
v8BreakIterator = Object().toJSBox;
browserSupportsImageDecoder = false;
// TODO(mdebbar): we don't check image codecs for now.
// https://github.com/flutter/flutter/issues/122331
Expand Down Expand Up @@ -1935,13 +1935,13 @@ void _paragraphTests() {


@JS('window.Intl.v8BreakIterator')
external dynamic get v8BreakIterator;
external JSAny? get v8BreakIterator;

@JS('window.Intl.v8BreakIterator')
external set v8BreakIterator(dynamic x);
external set v8BreakIterator(JSAny? x);

@JS('window.Intl.Segmenter')
external dynamic get intlSegmenter;
external JSAny? get intlSegmenter;

@JS('window.Intl.Segmenter')
external set intlSegmenter(dynamic x);
external set intlSegmenter(JSAny? x);
14 changes: 11 additions & 3 deletions lib/web_ui/test/engine/app_bootstrap_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,17 @@ void testMain() {

final FlutterEngineInitializer engineInitializer = bootstrap.prepareEngineInitializer();

final Object appInitializer = await promiseToFuture<Object>(callMethod<Object>(engineInitializer, 'initializeEngine', <Object?>[]));
final Object maybeApp = await promiseToFuture<Object>(callMethod<Object>(appInitializer, 'runApp', <Object?>[]));

final Object appInitializer = await promiseToFuture<Object>(callMethod<Object>(
engineInitializer,
'initializeEngine',
<Object?>[]
));
expect(appInitializer, isA<FlutterAppRunner>());
final Object maybeApp = await promiseToFuture<Object>(callMethod<Object>(
appInitializer,
'runApp',
<Object?>[]
));
expect(maybeApp, isA<FlutterApp>());
expect(initCalled, 1, reason: 'initEngine should have been called.');
expect(runCalled, 2, reason: 'runApp should have been called.');
Expand Down