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

Add ArrayBuffer fallback for #7918 #7983

Closed
wants to merge 3 commits into from
Closed

Add ArrayBuffer fallback for #7918 #7983

wants to merge 3 commits into from

Conversation

bspeice
Copy link

@bspeice bspeice commented Sep 3, 2018

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
Copy link

jsf-clabot commented Sep 3, 2018

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
Copy link
Contributor

For maintainers only:

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

Copy link
Member

@xtuc xtuc left a comment

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.

lib/wasm/WasmMainTemplatePlugin.js Show resolved Hide resolved
@webpack-bot
Copy link
Contributor

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

@bspeice
Copy link
Author

bspeice commented Sep 9, 2018

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
Copy link
Member

xtuc commented Sep 9, 2018

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

@bspeice
Copy link
Author

bspeice commented Sep 10, 2018

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

@bspeice
Copy link
Author

bspeice commented Sep 17, 2018

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

Copy link
Member

@xtuc xtuc left a comment

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?

@TheLarkInn
Copy link
Member

@xtuc How's this looking. Passes tests.

Copy link
Member

@xtuc xtuc left a comment

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.

lib/wasm/WasmMainTemplatePlugin.js Outdated Show resolved Hide resolved
@bspeice
Copy link
Author

bspeice commented Feb 4, 2019

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
Copy link
Author

bspeice commented Jul 24, 2020

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants