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

Document wasm JS builtins #37201

Merged
merged 51 commits into from
Dec 20, 2024

Conversation

chrisdavidmills
Copy link
Contributor

@chrisdavidmills chrisdavidmills commented Dec 13, 2024

Description

Chrome 130 supports WebAssembly JS String builtins.

Specifically, This PR adds:

  • A guide explaining what builtins are and how to use them.
  • Information about the new compileOptions parameter used to enable these built-ins, which is available to:
    • WebAssembly.compile()
    • WebAssembly.compileStreaming()
    • WebAssembly.instantiate()
    • WebAssembly.instantiateStreaming()
    • WebAssembly.validate()
    • The WebAssembly.Module() constructor

Motivation

Additional details

See https://chromestatus.com/feature/6695587390423040 for the data source.

Related issues and pull requests

@chrisdavidmills chrisdavidmills requested review from a team as code owners December 13, 2024 09:53
@chrisdavidmills chrisdavidmills requested review from hamishwillee and removed request for a team December 13, 2024 09:53
@github-actions github-actions bot added the Content:wasm WebAssembly docs label Dec 13, 2024
@chrisdavidmills chrisdavidmills marked this pull request as draft December 13, 2024 09:53
@github-actions github-actions bot added the size/m [PR only] 51-500 LoC changed label Dec 13, 2024
Copy link
Contributor

github-actions bot commented Dec 13, 2024

Preview URLs (9 pages)
External URLs (11)

URL: /en-US/docs/WebAssembly/Imported_string_constants
Title: WebAssembly Imported global string constants


URL: /en-US/docs/WebAssembly/JavaScript_builtins
Title: WebAssembly JavaScript builtins


URL: /en-US/docs/WebAssembly/JavaScript_interface/compile_static
Title: WebAssembly.compile()


URL: /en-US/docs/WebAssembly/JavaScript_interface/compileStreaming_static
Title: WebAssembly.compileStreaming()


URL: /en-US/docs/WebAssembly/JavaScript_interface/instantiateStreaming_static
Title: WebAssembly.instantiateStreaming()


URL: /en-US/docs/WebAssembly/JavaScript_interface/validate_static
Title: WebAssembly.validate()


URL: /en-US/docs/WebAssembly/JavaScript_interface/instantiate_static
Title: WebAssembly.instantiate()


URL: /en-US/docs/WebAssembly/JavaScript_interface/Module/Module
Title: WebAssembly.Module() constructor

(comment last updated: 2024-12-20 09:34:21)

@chrisdavidmills chrisdavidmills marked this pull request as ready for review December 13, 2024 13:32
Copy link
Contributor

@tomayac tomayac left a comment

Choose a reason for hiding this comment

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

LGMT, with just a couple of typo nits. Generally I think it should be "Wasm", no "wasm".

files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
chrisdavidmills and others added 13 commits December 16, 2024 11:42
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
Co-authored-by: Thomas Steiner <tomac@google.com>
```

> [!NOTE]
> In many cases it is not necessary to feature detect builtins. Another option could be to provide regular imports alongside the builtins, and supporting browsers will just ignore the fallbacks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Demonstrate this please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were going to add a separate section or article in the new year but with your recommendation to move the feature detection example to the main article, I might try to include this too. But I might still leave it to the new year as a separate task so we can get this merged before you disappear for your holidays ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, leaving it until the new year.


### Feature detecting WebAssembly JavaScript builtins

To write feature detection code for builtins, a different strategy is needed. When this feature is present, type checks will be stricter than when they are not present — certain rules are imposed on the builtin imports.
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I like this section - I didn't understand it when I scan-read the explainer. Seems a painful requirement to have to write your own testing function like this when everyone will have to do it.

Choose a reason for hiding this comment

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

But as line 119 says, most people won't need this. (And those that do can copy-paste these few lines?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus, people write their own feature detection code all the time or copy and paste it from MDN or SO. So I don't think this is anything out of the ordinary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re the testing function I think I was confused by why we said they won't need this - I think you mean "there are alternatives".

I guess my take was that it is a lot harder than feature checking against existence of a Javascript property on a prototype. It isn't all that onerous though.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

@chrisdavidmills Looks great. Some comment inline. I am around Friday and Monday if you want to iterate this to completion quickly.

chrisdavidmills and others added 6 commits December 18, 2024 08:40
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
…dex.md

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
…ndex.md

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
@github-actions github-actions bot added size/l [PR only] 501-1000 LoC changed and removed size/m [PR only] 51-500 LoC changed labels Dec 18, 2024
@chrisdavidmills
Copy link
Contributor Author

@chrisdavidmills Looks great. Some comment inline. I am around Friday and Monday if you want to iterate this to completion quickly.

@hamishwillee damn you and your pesky really useful review comments! (cheers mate, that was awesome). I've responded to all your stuff; hopefully, we can get this in before Xmas.

Copy link

@jakobkummerow jakobkummerow left a comment

Choose a reason for hiding this comment

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

LGTM with comments.

files/en-us/webassembly/imported_string_constants/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/imported_string_constants/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/imported_string_constants/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/imported_string_constants/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
files/en-us/webassembly/javascript_builtins/index.md Outdated Show resolved Hide resolved
Copy link

@jakobkummerow jakobkummerow left a comment

Choose a reason for hiding this comment

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

LGTM.

(I'll be out for the holidays now, so please don't wait for reviews from me for any future revisions.)

- `"wasm:js-string" "codePointAt"`
- : Equivalent to {{jsxref("String.prototype.codePointAt()")}}.
- `"wasm:js-string" "equals"`
- : Compares two string values for [strict equality](/en-US/docs/Web/JavaScript/Reference/Operators/Strict_equality), returning `1` if they are equal, and `0` if not.

Choose a reason for hiding this comment

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

Might be worth pointing out here that "equals" is the only function of the bunch that doesn't throw/trap for null inputs, so Wasm modules don't need to separately check for non-null-ness before calling "equals". (All the other functions have no reasonable way to handle null inputs, so they throw for them.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

```

> [!NOTE]
> In many cases it is not necessary to feature detect builtins. Another option could be to provide regular imports alongside the builtins, and supporting browsers will just ignore the fallbacks.

Choose a reason for hiding this comment

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

I suppose this is where we should add a link to the polyfill (in a separate PR next year).

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 indeed!

Copy link
Collaborator

Choose a reason for hiding this comment

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

By "In many cases it is not necessary to feature detect builtins" I think you are actually saying "there are alternatives to feature checking" - right? If so, this is a little ambiguous - it could mean you don't have to because it doesn't matter if they work or not.

So maybe

There are alternatives to feature checking for builtin support. For example, you can polyfil or .xxxx ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like this approach. Updated.

@@ -0,0 +1,155 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a separate doc works well for me!

Arguably you don't need the justification/explanation of the old way - you could present this without the reasons the new ways is better and get the the "how you do it" much faster. I would have done it that way, but happy with this as well, because I kind of like knowing "why's". Either way, it is very clear.

FWIW part of the problem I had with this before reading the doc is the term "import", as in we import strings into the module.

When I think of import I think of something coming from somewhere else, so in the original mechanism we import strings defined in JavaScript. Here we magically import strings, but we define them in place.
Or to put it another way, the semantics from a WASM point of view are import, but from a developer point of view it is define.

I don't know if that concern/misunderstanding makes sense, but might be nice to say a few words, if possible. Could be something that no one in the WASM space would have a problem with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I feel like this is a bit of an overwrought worry — if you don't care about the old way, and/or know it well, then you can just skip ahead to the second section. But the first section does help relative newcomers understand why this feature is needed (this was literally the first question in my mind when I started to learn about it), including why this is semantically still importing.

@hamishwillee
Copy link
Collaborator

@chrisdavidmills (and @jakobkummerow ) there are a few grumbles inline, but I think this is really good. What's your roadmap for publishing given the sidebar isn't present yet?

Or to put it another way, I can see your responses on (my) Monday. If I'm happy do you want me to approve/merge?

@chrisdavidmills
Copy link
Contributor Author

@chrisdavidmills (and @jakobkummerow ) there are a few grumbles inline, but I think this is really good. What's your roadmap for publishing given the sidebar isn't present yet?

Or to put it another way, I can see your responses on (my) Monday. If I'm happy do you want me to approve/merge?

@hamishwillee the new sidebar code got merged yesterday, so we are all good there.

I agreed with most of your last few comments and made the changes. I don't think there is anything controversial left on the table, so I'm gonna merge it now. Thanks all!

@chrisdavidmills chrisdavidmills merged commit ac338a2 into mdn:main Dec 20, 2024
8 checks passed
@chrisdavidmills chrisdavidmills deleted the wasm-js-string-builtins branch December 20, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:wasm WebAssembly docs size/l [PR only] 501-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants