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

Output .js files as ES6 modules. #52023

Merged
merged 11 commits into from
Jul 1, 2024
Merged

Conversation

eyebrowsoffire
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire commented Apr 10, 2024

This changes CanvasKit and Skwasm to be compiled and loaded as ES6 modules instead of as vanilla script tags. Currently, the emscripten JS files try to register themselves with require.js or AMD module loading systems. We suspect this is causing issues (flutter/flutter#149565) with DDC's module loading system, which itself uses require.js.

This is probably also the fix for flutter/flutter#147731

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Apr 10, 2024
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.

I don't think we need window.flutterCanvasKitLoaded, but that might be a change for another time. LGTM!

} catch (e) {
reject(e);
}
const canvasKitModule = await import(canvasKitUrl);
Copy link
Member

Choose a reason for hiding this comment

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

This is beautiful!

lib/web_ui/flutter_js/src/canvaskit_loader.js Show resolved Hide resolved
lib/web_ui/flutter_js/src/canvaskit_loader.js Outdated Show resolved Hide resolved
DEPS Outdated
@@ -277,7 +277,7 @@ allowed_hosts = [
]

deps = {
'src': 'https://github.com/flutter/buildroot.git' + '@' + '9a4ba8138aed94000ac5070590a21030008903bb',
'src': 'https://github.com/flutter/buildroot.git' + '@' + 'ef1bb419537f7bb16d4b7df0ed7a5e6f57a8d1fe',
Copy link
Member

Choose a reason for hiding this comment

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

(This file is conflicting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's because the buildroot thing hasn't actually landed yet. Once we are good to go to land everything, I'll actually land the buildroot one first, and then update this and there won't be a conflict.

lib/web_ui/flutter_js/src/canvaskit_loader.js Show resolved Hide resolved
lib/web_ui/flutter_js/src/skwasm_loader.js Show resolved Hide resolved
return js_util.promiseToFuture<CanvasKit>(
_CanvasKitInit(options).toObjectShallow);
extension CanvasKitModuleExtension on CanvasKitModule {
@JS('default')
Copy link
Member

Choose a reason for hiding this comment

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

🔪 💔

auto-submit bot pushed a commit to flutter/buildroot that referenced this pull request Jul 1, 2024
@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 1, 2024
@auto-submit auto-submit bot merged commit 91856e7 into flutter:main Jul 1, 2024
33 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 1, 2024
@eyebrowsoffire eyebrowsoffire added the revert Label used to revert changes in a closed and merged pull request. label Jul 1, 2024
Copy link
Contributor

auto-submit bot commented Jul 1, 2024

A reason for requesting a revert of flutter/engine/52023 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jul 1, 2024
@eyebrowsoffire
Copy link
Contributor Author

Reason for revert: Causing issues in engine -> framework roll flutter/flutter#151139

@eyebrowsoffire eyebrowsoffire added the revert Label used to revert changes in a closed and merged pull request. label Jul 1, 2024
auto-submit bot pushed a commit that referenced this pull request Jul 1, 2024
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jul 1, 2024
auto-submit bot added a commit that referenced this pull request Jul 1, 2024
Reverts: #52023
Initiated by: eyebrowsoffire
Reason for reverting: Causing issues in engine -> framework roll flutter/flutter#151139
Original PR Author: eyebrowsoffire

Reviewed By: {ditman}

This change reverts the following previous change:
This changes CanvasKit and Skwasm to be compiled and loaded as ES6 modules instead of as vanilla script tags. Currently, the emscripten JS files try to register themselves with require.js or AMD module loading systems. We suspect this is causing issues (flutter/flutter#149565) with DDC's module loading system, which itself uses require.js.

This is probably also the fix for flutter/flutter#147731
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 1, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 2, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 2, 2024
…151150)

flutter/engine@3456fee...fc5bc14

2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Output .js files as ES6 modules. (#52023)" (flutter/engine#53674)
2024-07-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 21c08743ee4a to c23e58143793 (1 revision) (flutter/engine#53670)
2024-07-01 skia-flutter-autoroll@skia.org Roll Skia from ae2b97d74812 to 8375bdc6e191 (3 revisions) (flutter/engine#53669)
2024-07-01 jacksongardner@google.com Output .js files as ES6 modules. (flutter/engine#52023)
2024-07-01 skia-flutter-autoroll@skia.org Roll Skia from a62bf018429c to ae2b97d74812 (3 revisions) (flutter/engine#53668)
2024-07-01 fmil@google.com [icu] Ignores the dir `flutter/third_party/icu/patches` (flutter/engine#53667)
2024-07-01 jonahwilliams@google.com [Impeller] track the sizes of all outstanding MTLTexture allocations and report per frame in MB, matching Vulkan implementation. (flutter/engine#53618)
2024-07-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 338c6d4fd9c5 to 21c08743ee4a (1 revision) (flutter/engine#53666)

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 jimgraham@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://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
sigurdm pushed a commit to sigurdm/flutter that referenced this pull request Jul 2, 2024
…lutter#151150)

flutter/engine@3456fee...fc5bc14

2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Output .js files as ES6 modules. (flutter#52023)" (flutter/engine#53674)
2024-07-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 21c08743ee4a to c23e58143793 (1 revision) (flutter/engine#53670)
2024-07-01 skia-flutter-autoroll@skia.org Roll Skia from ae2b97d74812 to 8375bdc6e191 (3 revisions) (flutter/engine#53669)
2024-07-01 jacksongardner@google.com Output .js files as ES6 modules. (flutter/engine#52023)
2024-07-01 skia-flutter-autoroll@skia.org Roll Skia from a62bf018429c to ae2b97d74812 (3 revisions) (flutter/engine#53668)
2024-07-01 fmil@google.com [icu] Ignores the dir `flutter/third_party/icu/patches` (flutter/engine#53667)
2024-07-01 jonahwilliams@google.com [Impeller] track the sizes of all outstanding MTLTexture allocations and report per frame in MB, matching Vulkan implementation. (flutter/engine#53618)
2024-07-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 338c6d4fd9c5 to 21c08743ee4a (1 revision) (flutter/engine#53666)

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 jimgraham@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://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
eyebrowsoffire added a commit to eyebrowsoffire/engine that referenced this pull request Jul 2, 2024
auto-submit bot pushed a commit that referenced this pull request Jul 3, 2024
This is an attempt to reland #52023. The issue previously is that if it was not specified by the user, the default CanvasKit base URL did not have a leading slash, which does not work when doing dynamic imports.
auto-submit bot pushed a commit that referenced this pull request Jul 3, 2024
auto-submit bot added a commit that referenced this pull request Jul 3, 2024
…#53709)

Reverts: #53688
Initiated by: jiahaog
Reason for reverting: canvaskit.js cannot be loaded in an internal end to end test - see b/350885206
Original PR Author: eyebrowsoffire

Reviewed By: {ditman}

This change reverts the following previous change:
This is an attempt to reland #52023. The issue previously is that if it was not specified by the user, the default CanvasKit base URL did not have a leading slash, which does not work when doing dynamic imports.
eyebrowsoffire added a commit to eyebrowsoffire/engine that referenced this pull request Jul 3, 2024
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
…lutter#151150)

flutter/engine@3456fee...fc5bc14

2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Output .js files as ES6 modules. (flutter#52023)" (flutter/engine#53674)
2024-07-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 21c08743ee4a to c23e58143793 (1 revision) (flutter/engine#53670)
2024-07-01 skia-flutter-autoroll@skia.org Roll Skia from ae2b97d74812 to 8375bdc6e191 (3 revisions) (flutter/engine#53669)
2024-07-01 jacksongardner@google.com Output .js files as ES6 modules. (flutter/engine#52023)
2024-07-01 skia-flutter-autoroll@skia.org Roll Skia from a62bf018429c to ae2b97d74812 (3 revisions) (flutter/engine#53668)
2024-07-01 fmil@google.com [icu] Ignores the dir `flutter/third_party/icu/patches` (flutter/engine#53667)
2024-07-01 jonahwilliams@google.com [Impeller] track the sizes of all outstanding MTLTexture allocations and report per frame in MB, matching Vulkan implementation. (flutter/engine#53618)
2024-07-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 338c6d4fd9c5 to 21c08743ee4a (1 revision) (flutter/engine#53666)

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 jimgraham@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://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
auto-submit bot pushed a commit that referenced this pull request Jul 10, 2024
Second attempt to reland #52023

Fixes since the previous reland attempt:
* We need to pass the skwasm main JS URI when loading the module so that it can pass that along to the worker. Since the worker uses the workaround to allow a cross script worker, it has trouble locating the main JS URI in relation to itself in a way that actually works for dynamic imports, so passing it along fixes that issue.
* Some of the Google3 tests relied on the relative default canvaskit path. Dynamic module imports seems to not handle relative paths the way we expect, so we do our own URL resolution using the URL constructor before passing it into the dynamic import API. Also cleaned up some of the other relative pathing stuff that we do around the base URI. in flutter.js
flutteractionsbot pushed a commit to flutteractionsbot/engine that referenced this pull request Aug 8, 2024
…er#53718)

Second attempt to reland flutter#52023

Fixes since the previous reland attempt:
* We need to pass the skwasm main JS URI when loading the module so that it can pass that along to the worker. Since the worker uses the workaround to allow a cross script worker, it has trouble locating the main JS URI in relation to itself in a way that actually works for dynamic imports, so passing it along fixes that issue.
* Some of the Google3 tests relied on the relative default canvaskit path. Dynamic module imports seems to not handle relative paths the way we expect, so we do our own URL resolution using the URL constructor before passing it into the dynamic import API. Also cleaned up some of the other relative pathing stuff that we do around the base URI. in flutter.js
auto-submit bot pushed a commit that referenced this pull request Aug 14, 2024
…54446)

This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request)
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.

### Issue Link:
What is the link to the issue this cherry-pick is addressing?

- flutter/flutter#152953
- flutter/flutter#152844

### Changelog Description:

Fixes the loading of CanvasKit in a way that avoids app crashes and timeout issues.

### Impact Description:

When CanvasKit is downloaded from the network (not cached), there's a high likelihood that the app crashes and displays a blank screen.

### Workaround:

If the browser cache is enabled, users can refresh the page to get CanvasKit from cache and avoid the issue.

### Risk:
What is the risk level of this cherry-pick?

### Test Coverage:
Are you confident that your fix is well-tested by automated tests?

### Validation Steps:

- Run a sample flutter app using `--web-renderer=canvaskit`.
- In Chrome's Network tab, check the `Disable cache` checkbox.
- Reload the app a few times and make sure it's working and there are no errors in the console.
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
2 participants