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

Update dart2wasm module runner #2286

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Sep 30, 2024

Update dart2wasm module instantiation, compilation, and invoking main to support all the ways dart2wasm implemented.

With [1] dart2wasm got a new way of compiling and instantiation modules.

Update test runner to use the new API.

[1]: dart-lang/sdk@f98a84a
@osa1 osa1 requested a review from a team as a code owner September 30, 2024 09:02
Copy link

github-actions bot commented Sep 30, 2024

PR Health

Package publish validation ✔️
Package Version Status
package:checks 0.3.1-wip WIP (no publish necessary)
package:test 1.25.9-wip WIP (no publish necessary)
package:test_api 0.7.4-wip WIP (no publish necessary)
package:test_core 0.6.6-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@osa1
Copy link
Member Author

osa1 commented Sep 30, 2024

Do we need to update some min. SDK version in this repo to the version with the new API?

I think the test runner is shipped with the SDK (instead of fetched from pub.dev), so maybe we just need to roll this change into the SDK?

Also, does this need a changelog entry? If the runner is shipped with the SDK then I think a changelog is not necessary, because the change is not visible by the users.

@osa1
Copy link
Member Author

osa1 commented Sep 30, 2024

Note: we are bringing back the deprecated API in https://dart-review.googlesource.com/c/sdk/+/387162.

We should still merge this change in prep. for the removal of the deprecated API.

@jakemac53
Copy link
Contributor

It looks like we need to update the min SDK constraints to 3.6.0-271.0.dev. @osa1 did you want me to push a change to this PR to do that across the repo?

@jakemac53
Copy link
Contributor

Given that the new API isn't available in a stable SDK yet we should also discuss whether we should wait to land and/or publish this until 3.6 is out.

When do we plan to remove the deprecated API again?

@natebosch
Copy link
Member

I think the test runner is shipped with the SDK (instead of fetched from pub.dev), so maybe we just need to roll this change into the SDK?

The test runner is fetched from pub to allow changes to the user code without breaking SDK changes.

let dart2wasm = await import('./' + data.jsruntimeurl);
let dartInstance = await dart2wasm.instantiate(modulePromise, {});
let dart2wasmJsRuntime = await import('./' + data.jsruntimeurl);
let compiledModule = await dart2wasmJsRuntime.compileStreaming(fetch(data.wasmurl));
Copy link
Member

Choose a reason for hiding this comment

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

Can we check whether an instantiate method exists and choose at runtime which approach to use?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would definitely be ideal, so we can still support 3.5 SDKs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I've updated this to support both APIs.

I run dart test in this package with the latest stable SDK and the main branch (before reverting the removal of the deprecated API). In both cases I got the same test results, except there was one more failure with the main branch but the test seemed unrelated (it mentions VM specific things).

@jakemac53
Copy link
Contributor

Note https://github.com/dart-lang/test/pull/2285/files is skipping some of these for now, we should make sure to unskip them when landing this.

@osa1
Copy link
Member Author

osa1 commented Oct 1, 2024

It looks like we need to update the min SDK constraints to 3.6.0-271.0.dev. @osa1 did you want me to push a change to this PR to do that across the repo?

Please feel free to always make any changes you think are necessary to my branches, this one and in the future as well.

When do we plan to remove the deprecated API again?

Whenever the users are ready. The new API allows deferred loading, but it cannot be used in package:test tests anyway as you have to call JS instantiate method yourself to provide a callback to fetch the deferred modules.

Note https://github.com/dart-lang/test/pull/2285/files is skipping some of these for now, we should make sure to unskip them when landing this.

I'm not sure what that PR is fixing exactly. It refers to a 2 year old issue that should be invalid now as dart2wasm is quite stable today compared to two years ago.

@osa1 osa1 requested review from natebosch and jakemac53 October 1, 2024 11:35
@osa1 osa1 changed the title Update dart2wasm module instantiation Update dart2wasm module runner Oct 1, 2024
@jakemac53
Copy link
Contributor

I'm not sure what that PR is fixing exactly. It refers to a 2 year old issue that should be invalid now as dart2wasm is quite stable today compared to two years ago.

It was just adding additional skips to get the bots green and linking re-using an old issue for tracking that.

// browser's `WebAssembly` API, the compiled module needs to be instantiated
// using the JS runtime.
//
// (2) Versions starting with 3.6.0-167.0.dev added helpers for compiling and
Copy link
Member

Choose a reason for hiding this comment

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

It would be OK to drop support for a strategy that was only in a series of dev releases, but supporting it sounds good too.

After we have any other reason to bump the min SDK we will get this cleaned up to only the version 3 approach.

@natebosch natebosch mentioned this pull request Oct 1, 2024
@natebosch natebosch merged commit c98c6e3 into dart-lang:master Oct 1, 2024
54 checks passed
@osa1 osa1 deleted the wasm_loader branch October 2, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants