Skip to content

Add ArrayBuffer fallback for #7918 - #7983

Closed
bspeice wants to merge 3 commits into
webpack:masterfrom
bspeice:patch-1
Closed

Add ArrayBuffer fallback for #7918#7983
bspeice wants to merge 3 commits into
webpack:masterfrom
bspeice:patch-1

Conversation

@bspeice

@bspeice bspeice commented Sep 3, 2018

Copy link
Copy Markdown

What kind of change does this PR introduce?

Adds a failure case so Electron-style apps can easily use WebAssembly

Did you add tests for your changes?

Doesn't run a headless test, not sure how best to add test support (is tested using a local Electron app)

Does this PR introduce a breaking change?

No

What needs to be documented once your changes are merged?

Hopefully nothing? Think there may be a README example that needs updating, but this should be something that "just fixes" some background issues.

@jsf-clabot

jsf-clabot commented Sep 3, 2018 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@webpack-bot

webpack-bot commented Sep 3, 2018

Copy link
Copy Markdown
Contributor

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

xtuc
xtuc reviewed Sep 3, 2018
View reviewed changes

@xtuc xtuc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's a great solution.

In the issue I thought you wanted to inline the binary, which I would be against to.

Comment thread lib/wasm/WasmMainTemplatePlugin.js Resolved
@webpack-bot

webpack-bot commented Sep 3, 2018

Copy link
Copy Markdown
Contributor

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

@bspeice

bspeice commented Sep 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

Alright, continued working with this, and bad news - this patch works fine for --mode=development, but --mode=production has issues:

CompileError: WasmCompile: Wasm decoding failed: field name: no valid UTF-8 string @+1791
    at file:///home/bspeice/Development/isomorphic_rust/percy_patched_webpack/dist/bundle.js:1:2658
    at <anonymous>

Character 2658 is the actual call to WebAssembly.instantiate, and character 1791 doesn't seem to be relevant anywhere. Is this something that should still go through?

@xtuc

xtuc commented Sep 9, 2018

Copy link
Copy Markdown
Contributor

@bspeice could you please try again with this patch #7732?

@bspeice

bspeice commented Sep 10, 2018

Copy link
Copy Markdown
Author

Can confirm that #7732 does in fact fix the issues I saw. --mode=production runs fine on that version.

@bspeice

bspeice commented Sep 17, 2018

Copy link
Copy Markdown
Author

@xtuc - Anything further needed on this? Everything seems to be working from my end, want to make sure I didn't miss something.

xtuc
xtuc reviewed Sep 18, 2018
View reviewed changes

@xtuc xtuc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please add tests on that?

I guess you can overide the WebAssembly.instantiateStreaming function to make it throw and test the fallback?

@xtuc xtuc mentioned this pull request Sep 30, 2018
Closed
@TheLarkInn

TheLarkInn commented Dec 2, 2018

Copy link
Copy Markdown
Member

@xtuc How's this looking. Passes tests.

xtuc
xtuc suggested changes Dec 3, 2018
View reviewed changes

@xtuc xtuc left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@TheLarkInn do we know if that's a common issue in pratice?

I'm not personally blocking it, but i'm not happy with the solution. There are many reasons for TypeErrors during instantiateStreaming.

However, the worst case would be to call instantiate twice. I'm pretty sure it won't run two compilations, most of the type mismatch are coming from the ImportObject, which is checked before.

Ideally, either Electron should figure out a way to correctly load Wasm or the mime-type restriction needs to change.

Comment thread lib/wasm/WasmMainTemplatePlugin.js Resolved
@bspeice

bspeice commented Feb 4, 2019

Copy link
Copy Markdown
Author

Given that both Electron is unlikely to substantively change Blink to allow this, and the spec is unlikely to change, is this option acceptable? If not, I'd rather close out the request for now to clean things up.

@bspeice

bspeice commented Jul 24, 2020

Copy link
Copy Markdown
Author

As noted in #7918, this is fixed simply by using Webpack 5 (and enabling the asyncWebAssembly experiment). This change is no longer necessary.

@bspeice bspeice closed this Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@xtuc xtuc xtuc requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@bspeice @jsf-clabot @webpack-bot @xtuc @TheLarkInn @ooflorent