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

Top-level-await, new WASM modules and async modules based on import await and import async #9177

Merged
merged 19 commits into from Jun 5, 2019

Conversation

@sokra
Copy link
Member

sokra commented May 22, 2019

  • adds top-level-await support
  • adds import await support
  • adds import async support
  • adds exports await from support
  • adds exports from async support
  • adds new async wasm support based on that
  • make wasm and top-level-await experiements

see example: https://github.com/webpack/webpack/tree/feature/top-level-await/examples/top-level-await

cc @littledan @jhnns @lukastaegert @TheLarkInn

based on https://gist.github.com/sokra/33f3db1b2714e9a6720f890842c47ae6

What kind of change does this PR introduce?
feature

Did you add tests for your changes?
yes

Does this PR introduce a breaking change?
yes, wasm is now opt-in by default

What needs to be documented once your changes are merged?
New experiments:

  • experiments.importAwait -> import/export await is enabled
  • experiments.importAsync -> import/export allows to use async module too
  • experiments.topLevelAwait -> allows to use await on top-level
  • experiments.asyncWebAssembly -> WebAssembly Modules are async modules
  • experiments.syncWebAssembly -> The webpack 4 WebAssembly Modules implementation (outdated)
@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented May 22, 2019

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)
@sokra sokra changed the title WIP prototype of top-level-await and async modules based on import await Prototype of top-level-await and async modules based on import await May 22, 2019
@sokra sokra force-pushed the feature/top-level-await branch from 8d56afb to fae4598 May 23, 2019
@littledan littledan mentioned this pull request May 23, 2019
0 of 2 tasks complete
@sokra sokra marked this pull request as ready for review May 24, 2019
@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented May 24, 2019

So I added WASM-ESM-integration based on WASM being async modules according to the proposal here: https://github.com/WebAssembly/esm-integration/tree/master/proposals/esm-integration

Here are a few examples:

There are a few functional changes compared to the old wasm implementation:

  • Importing memory, tables and i64-siguratures from JS is now allowed. Now there are no additional restrictions compared to the native WASM API. Actually this is used without change.
  • As circular dependencies are not allowed in async modules, circular dependencies including a WASM module are not supported. It was allowed before because trampolines are used for functions.
  • WASM need to be imported with import await or import(). This was not required before.
  • WASM can be in the initial chunk.
  • require() any async module gives a Promise, so does require-ing WASM now. This was the exports object before.

Implementation-wise:

  • No function trampolines are generated for wasm (Better runtime performance)
  • WASM does no longer be rewritten. (Better build performance at least in dev mode)
  • The new wasm implementation is default, but the old can still be enabled with type: "webassembly/experimental". But I would consider it deprecated and it will be removed in next major.

A note on wasm-bindgen: It will break for two reasons:

  • It doesn't use import await to import the wasm part. This can easily changed.
  • The wasm part imports the js part in a circular way. As circular dependencies are not supported, it need to be changed to generate 3 files instead. 1 WASM part and 2 JS parts.
@sokra sokra added this to Probably in webpack 5 May 24, 2019
@sokra sokra force-pushed the feature/top-level-await branch from 214e645 to 544d3e1 May 24, 2019
@sokra sokra changed the title Prototype of top-level-await and async modules based on import await Top-level-await, new WASM modules and async modules based on import await May 24, 2019
@webpack-bot webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels May 24, 2019
@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented May 29, 2019

Wow @sokra that's an awesome update! Thanks for the work.

From a Wasm viewpoint it's mostly lgtm, apart from:

i64-siguratures from JS is now allowed

This should still be disallowed, as it will throw when called from JS. A proposal exists already for that: https://github.com/WebAssembly/JS-BigInt-integration.

Do you think this can still land this week?

@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented May 31, 2019

This should still be disallowed, as it will throw when called from JS. A proposal exists already for that: https://github.com/WebAssembly/JS-BigInt-integration.

Before this change we throw a build error when using i64-signatures, but as this will be soonish supported, I feel a build error is to much. Using an i64-signature might not even be an error, as somebody could have a browser with BigInt support.

@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented May 31, 2019

Do you think this can still land this week?

Not sure. I actually wait for some feedback from @littledan about the import vs import await topic in the top-level-await discussion.

Unrelated from this: I want to move this and other things behind experiments flags, so you need to opt-in to use experimental features:

experiments: {
  mjs: true,
  topLevelAwait: true,
  asyncWasm: true,
  // ...
}
@littledan

This comment has been minimized.

Copy link
Contributor

littledan commented May 31, 2019

Opt in sounds important to do before landing this. I am excited to see the Webpack leading with experimentation here. However, the current TC39 proposal is based on import, not import await.

@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented Jun 3, 2019

However, the current TC39 proposal is based on import, not import await.

I could also implement this, but it's a bit more involved since the async-or-not status of a module no longer only depends on itself, but on all dependencies too. So async status need to be inferred in a separate step after building the module graph. (That's also what I'm criticizing on the proposal. This is confusing for the developer too)

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Jun 3, 2019

Before this change we throw a build error when using i64-signatures, but as this will be soonish supported, I feel a build error is to much. Using an i64-signature might not even be an error, as somebody could have a browser with BigInt support.

That's very true, I agree.

Unrelated from this: I want to move this and other things behind experiments flags

Would that allow Webpack to iterate faster, aka break code?

@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented Jun 3, 2019

Would that allow Webpack to iterate faster, aka break code?

I would use relaxed SemVer for experiments so breaking changes in experiments could occur in minor versions.

@sokra sokra force-pushed the feature/top-level-await branch from 544d3e1 to f4c133f Jun 3, 2019
@MylesBorins

This comment has been minimized.

Copy link

MylesBorins commented Jun 5, 2019

Hey All,

We put together a small doc as an explainer for why the champions of Top-Level Await are not planning to move forward with the import await syntax for today. FWIW this is in no way implying that this PR shouldn't land, that tooling shouldn't experiment with new ideas, or that this proposal couldn't come to the committee in the future. I've included a link to the gist and the important bits copy pasted below.

Thank you for thanking the time to work on this alternative approach, it really helped us improve the algorithm for Top-Level Await.

https://gist.github.com/MylesBorins/ae97abab4a4144411c12240bb09dc7dd


How the current specification works

  • No more microticks unless there is top-level await
    • If no top-level await in graph then it is executed synchronously
  • Optmizes async execution
    • Only the modules that include a Top-Level Await are run asyncronously
    • Subsquent sub graphs are run synchronously
    • No micro ticks for remaining graph

Concerns with import await

  • Viral Syntax
    • no way to introduce async module into graph without import await to root
    • use of import await defensively likely to happen
  • wasm modules will be async
    • will force introduction of wasm modules to be Semver-Major (and viral)
  • Removes ability for engine to optimize async execution
    • import await would force graph up to root to execute asynchronously
    • running import async modules synchronously would break developer contract

In Summary

  • It is my belief that import await is not a replacement for Top-Level Await
  • This does not mean that others, e.g. webpack, should not experiment with the API
  • this does not preclude import await from being presented to the committee as an additional proposal in the future
@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented Jun 5, 2019

@MylesBorins Here is my answer to this: https://gist.github.com/sokra/34d2afe7555eb9a687e2c7566e9a9646

I'm currently in progress of implementing the original top-level-await proposal in this PR in addition to import await. You will be able to experiment with both.

I'm currently pushing towards import await because I favor it over import, but if the final spec will have a different shape I'm happy to use this instead. Both proposals solve a lot of problems with wasm modules we currently have. Having any of it is great.

One thing I have a bit trouble with is understanding the exact behavior of async cycles. I would love to hear some plaintext explanation about that.

@littledan

This comment has been minimized.

Copy link
Contributor

littledan commented Jun 5, 2019

To clarify, cycles of modules which have static import statements do not deadlock. The cycle is broken just as today with sync modules.

@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented Jun 5, 2019

// entry.js
import x from "a";
import y from "b";
console.log(x);
console.log(y);

// a.js
import { hello, world } from "b";
export default hello + world;
await 1;
export function f() {
  return "hello";
}

// b.js
import value, { f } from "a";
export const hello = f();
await 1;
export const world = "world";
export default (() => { try { value } catch(e) { return e; } })();

So am I correct that this example prints:

  • helloworld
  • ReferenceError
@sokra sokra force-pushed the feature/top-level-await branch from f4c133f to a14c6fb Jun 5, 2019
@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented Jun 5, 2019

So both proposals are now supported on opt-in:

  • experiments.importAwait -> import/export await is enabled
  • experiments.importAsync -> import/export allows to use async module too
  • experiments.topLevelAwait -> allows to use await on top-level
  • experiments.asyncWebAssembly -> WebAssembly Modules are async modules
@webpack-bot webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Jun 5, 2019
@sokra sokra changed the title Top-level-await, new WASM modules and async modules based on import await Top-level-await, new WASM modules and async modules based on import await and import async Jun 5, 2019
@webpack-bot webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jun 5, 2019
@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Jun 5, 2019

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra force-pushed the feature/top-level-await branch from c752dd6 to 11fb0a9 Jun 5, 2019
@sokra sokra merged commit a71342b into next Jun 5, 2019
23 of 25 checks passed
23 of 25 checks passed
codecov/changes/basic 9 files have unexpected coverage changes not visible in diff.
Details
codecov/project/integration 89.82% (-10.18%) compared to 09f2fa7
Details
codecov/changes/integration No unexpected coverage changes found.
Details
codecov/changes/unit No unexpected coverage changes found.
Details
codecov/patch/basic 92.28% of diff hit (target 90%)
Details
codecov/patch/integration 92.91% of diff hit (target 90%)
Details
codecov/patch/unit Coverage not affected when comparing 09f2fa7...11fb0a9
Details
codecov/project/basic Absolute coverage decreased by -1.04% but relative coverage increased by +10.69% compared to 09f2fa7
Details
codecov/project/unit 100% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk - package.json (sokra) No manifest changes detected
webpack.webpack Build #20190605.6 succeeded
Details
webpack.webpack (Linux node-10) Linux node-10 succeeded
Details
webpack.webpack (Linux node-12) Linux node-12 succeeded
Details
webpack.webpack (Linux node-8) Linux node-8 succeeded
Details
webpack.webpack (Windows node-10) Windows node-10 succeeded
Details
webpack.webpack (Windows node-12) Windows node-12 succeeded
Details
webpack.webpack (Windows node-8) Windows node-8 succeeded
Details
webpack.webpack (basic) basic succeeded
Details
webpack.webpack (lint) lint succeeded
Details
webpack.webpack (macOS node-10) macOS node-10 succeeded
Details
webpack.webpack (macOS node-12) macOS node-12 succeeded
Details
webpack.webpack (macOS node-8) macOS node-8 succeeded
Details
@sokra sokra deleted the feature/top-level-await branch Jun 5, 2019
@sokra

This comment has been minimized.

Copy link
Member Author

sokra commented Jun 5, 2019

So this landed in 5.0.0-alpha.15

@sokra sokra moved this from Probably to Done in webpack 5 Jun 6, 2019
@robpalme robpalme mentioned this pull request Aug 1, 2019
2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.