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

module: unflag WASM modules #42309

Closed
wants to merge 1 commit into from
Closed

Conversation

gthb
Copy link

@gthb gthb commented Mar 12, 2022

WebAssembly has been unflagged in V8 for years, and is presented on equal footing with Javascript in how V8 describes itself for years now ("Google’s open source high-performance JavaScript and WebAssembly engine").

I can't find any specified reasons to keep it flagged and called experimental in Node (though maybe this PR will draw those out?).

Following #41736 as a model, this PR leaves the flag present as a no-op and removes it from documentation, but keeps the "Stability: 1 - Experimental" in esm.md and the runtime warning message. I want to remove those too (or else draw out some specified reasons not to :) ) but there appears to be a convention of decoupling unflagging from stabilizing: #41736 (comment), so, like JSON modules, maybe drop the warning in 18.0.0?

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Mar 12, 2022
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The WebAssembly ESM integration is still uncertain in some respects, and we should only land this once the full end-to-end workflows for WebAssembly in ESM are a little more clear. These use cases are possible without modules using direct instantiation APIs and since there is more risk to landing than in waiting I would oppose this, at least while things like the WebAssembly component model is being worked out (https://github.com/WebAssembly/component-model).

WebAssembly has been unflagged in V8 for years, and is presented on
equal footing with Javascript in [how V8 describes itself][v8.dev] for
years now ("Google’s open source high-performance JavaScript and
WebAssembly engine").

I can't find any specified reasons to keep it flagged and called
experimental in Node (though maybe this PR will draw those out?).

Following nodejs#41736 as a model, this PR
leaves the flag present as a no-op and removes it from documentation,
but keeps the "Stability: 1 - Experimental" in esm.md and the runtime
warning message.  I want to remove those too (or else draw out some
specified reasons not to :) ) but there appears to be a convention of
decoupling unflagging from stabilizing:
nodejs#41736 (comment), so,
like JSON modules, maybe drop the warning in 18.0.0?

[v8.dev]: https://v8.dev/
@gthb
Copy link
Author

gthb commented Mar 12, 2022

(That force-push was just to rewrite the commit message to satisfy the commit message linter)

@gthb
Copy link
Author

gthb commented Mar 12, 2022

@guybedford That's already some added clarity, thanks.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

We can't unflag wasm modules until the html spec clarifies whether an assertion will be required for import of wasm. If they decide to require one, we will too; so if we unflag now without one, adding it later will be a breaking change. See #40766

@GeoffreyBooth
Copy link
Member

@guybedford It seems like we already have a way to use WebAssembly in Node without needing --experimental-wasm-modules, don’t we? Looking at this as an example:

let wasm;
let initPromise;
export function init () {
  if (initPromise)
    return initPromise;
  return initPromise = (async () => {
    const compiled = await WebAssembly.compile(
      (binary => typeof window !== 'undefined' && typeof atob === 'function' ? Uint8Array.from(atob(binary), x => x.charCodeAt(0)) : Buffer.from(binary, 'base64'))
      ('WASM_BINARY')
    )
    const { exports } = await WebAssembly.instantiate(compiled);
    wasm = exports;
  })();
}

Should we perhaps put this or a similar example in our docs and remove --experimental-wasm-modules? Since this is non-experimental, and I would think that we should prefer/encourage stable patterns over experimental ones. It’s obviously a lot more verbose, but a library could hide the details I’d think.

And then once there’s some clarity from the WebAssembly and HTML specs over whether and how they might support import of Wasm, we can reintroduce it via a new flag like --experimental-wasm-imports.

@guybedford
Copy link
Contributor

@GeoffreyBooth we also have a good example of WASI usage in https://nodejs.org/dist/latest-v17.x/docs/api/wasi.html using a similar instantiation technique. Yes I would generally link to these examples over the --experimental-wasm-imports for now, but also keeping --experimental-wasm-imports in is important to show we are implementers of the ESM integration I think too.

@gthb
Copy link
Author

gthb commented Mar 14, 2022

we already have a way to use WebAssembly in Node without needing --experimental-wasm-modules, don’t we?

Right, this is where I was confused until @guybedford's first reply above. WebAssembly itself has been unflagged since Node v8.0.0 (it was enabled by default in V8 version 5.7). It's just the direct import as an ES module that's still flagged. I had misunderstood the flag name “experimental wasm modules” because “module” is also a thing in WebAssembly itself, but in the flag name, “modules” means “as ES modules”.

@GeoffreyBooth
Copy link
Member

keeping --experimental-wasm-modules in is important to show we are implementers of the ESM integration I think too.

I think it’s a tradeoff. Yes, by having a working implementation we help push the standardization effort forward; but we’ve had this for years now without any progress on that front. The downside is that we’re sending a message to our users that this is the likely way forward, which is probably the wrong message to send at this point. I’m not sure what’s gained by continuing to keep in Node an implementation that seems unlikely to be the way the standards go (?) versus removing it to prevent users from relying on this approach versus the WASI method.

@GeoffreyBooth
Copy link
Member

I think we can close this PR for now? I think it’s more likely that we remove --experimental-wasm-modules than unflag it, at least for the time being until there’s any progress on the Wasm-ESM integration effort.

@gthb
Copy link
Author

gthb commented Apr 3, 2022

I think we can close this PR for now? I think it’s more likely that we remove --experimental-wasm-modules than unflag it, at least for the time being until there’s any progress on the Wasm-ESM integration effort.

Is there something necessitating the removal? It would inconvenience those of us who started using it and have no problems. I do get that an experimental flag comes with no guarantees that it will stay in place, but I feel there ought to be some reason if it is to be removed.

@gthb gthb deleted the unflag-wasm-modules branch January 19, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants