Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@eyebrowsoffire
Copy link
Contributor

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

@eyebrowsoffire eyebrowsoffire changed the title Reland es6 2 Reland (x2) "Output .js files as ES6 modules. (flutter#52023)" Jul 3, 2024
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jul 3, 2024
@kevmoo
Copy link
Contributor

kevmoo commented Jul 3, 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.

LGTM!

return config.canvasKitBaseUrl;
}
if (buildConfig.engineRevision && !buildConfig.useLocalCanvasKit) {
return joinPathSegments("https://www.gstatic.com/flutter-canvaskit", buildConfig.engineRevision);
Copy link
Member

Choose a reason for hiding this comment

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

(The gstatic.com/flutter-canvaskit URL should come from buildConfig as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I agree. Although I'm not going to tackle that with this change (and would require other changes in the framework first).

Copy link
Member

Choose a reason for hiding this comment

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

100% agreed


String _resolveUrl(String url) {
return DomURL(url.toJS, domWindow.document.baseUri?.toJS).toJSString().toDart;
return createDomURL(url, domWindow.document.baseUri).toJSString().toDart;
Copy link
Member

Choose a reason for hiding this comment

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

You could wrap the .toJSString().toDart in a toString() method in the DomURLExtension :)

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2024
@auto-submit auto-submit bot merged commit 1c23c8f into flutter:main Jul 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 10, 2024
eyebrowsoffire added a commit to eyebrowsoffire/flutter that referenced this pull request Jul 10, 2024
Now that flutter/engine#53718 is fixed, these tests should be fine.
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 11, 2024
Now that flutter/engine#53718 is fixed, these tests should be fine.

After this, we should be able to close #147731
@eyebrowsoffire eyebrowsoffire added the cp: beta cherry pick to the beta release candidate branch label Jul 15, 2024
@flutteractionsbot
Copy link

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

@mdebbar mdebbar added cp: stable cherry pick to the stable release candidate branch and removed cp: beta cherry pick to the beta release candidate branch labels Aug 8, 2024
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App cp: stable cherry pick to the stable release candidate branch platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants