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

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

Merged
merged 3 commits into from Sep 12, 2023

Conversation

eyebrowsoffire
Copy link
Contributor

This prepares for some upcoming changes to dart2js which will be more strict about what types can be used in a JS interop declaration.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Sep 11, 2023
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.

Testing small nit about the configuration of the engine, otherwise LGTM!

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.

@@ -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.

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 12, 2023
@auto-submit auto-submit bot merged commit 14a858c into flutter:main Sep 12, 2023
25 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 13, 2023
…ions) (#134589)

Manual roll requested by bdero@google.com

flutter/engine@496ef6a...c90fadf

2023-09-12 skia-flutter-autoroll@skia.org Manual roll Dart SDK from 43d4b1373788 to 1ee7ef8bbc65 (6 revisions) (flutter/engine#45726)
2023-09-12 ychris@google.com Reland "Build iOS unittest target in unopt builds" (#44356)"" (#45346)" (flutter/engine#45519)
2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from a4f8f5177c8b to 211d63b1e1f5 (2 revisions) (flutter/engine#45724)
2023-09-12 jacksongardner@google.com Fix JS interop signatures to use only JS types. (flutter/engine#45668)
2023-09-12 ychris@google.com [ios] Fix testDeallocated failing locally.  (flutter/engine#45663)
2023-09-12 ychris@google.com [iOS] move arm64 builds to arm machines (flutter/engine#45721)

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 bdero@google.com,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
…ions) (flutter#134589)

Manual roll requested by bdero@google.com

flutter/engine@496ef6a...c90fadf

2023-09-12 skia-flutter-autoroll@skia.org Manual roll Dart SDK from 43d4b1373788 to 1ee7ef8bbc65 (6 revisions) (flutter/engine#45726)
2023-09-12 ychris@google.com Reland "Build iOS unittest target in unopt builds" (flutter#44356)"" (flutter#45346)" (flutter/engine#45519)
2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from a4f8f5177c8b to 211d63b1e1f5 (2 revisions) (flutter/engine#45724)
2023-09-12 jacksongardner@google.com Fix JS interop signatures to use only JS types. (flutter/engine#45668)
2023-09-12 ychris@google.com [ios] Fix testDeallocated failing locally.  (flutter/engine#45663)
2023-09-12 ychris@google.com [iOS] move arm64 builds to arm machines (flutter/engine#45721)

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 bdero@google.com,rmistry@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
4 participants