-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Parameter maybe_memory
should be optional
#2133
Comments
Thanks for the report, but I believe that this parameter is already optional and not required if memory isn't imported. It should only be required if the wasm module imports memory, in which case there's nothing we can do if it's not supplied. |
@alexcrichton Hmmm, but this is the generated code when multithreading is enabled: async function load(module, imports, maybe_memory) {
if (typeof Response === 'function' && module instanceof Response) {
memory = imports.wbg.memory = new WebAssembly.Memory({initial:17,maximum:16384,shared:true});
if (typeof WebAssembly.instantiateStreaming === 'function') {
try {
return await WebAssembly.instantiateStreaming(module, imports);
} catch (e) {
if (module.headers.get('Content-Type') != 'application/wasm') {
console.warn("`WebAssembly.instantiateStreaming` failed because your server does not serve wasm with `application/wasm` MIME type. Falling back to `WebAssembly.instantiate` which is slower. Original error:\n", e);
} else {
throw e;
}
}
}
const bytes = await module.arrayBuffer();
return await WebAssembly.instantiate(bytes, imports);
} else {
memory = imports.wbg.memory = maybe_memory;
const instance = await WebAssembly.instantiate(module, imports);
if (instance instanceof WebAssembly.Instance) {
return { instance, module };
} else {
return instance;
}
}
} So if you pass in a Module or ArrayBuffer then it won't work properly. So it should instead be something like this: async function load(module, imports, maybe_memory) {
if (maybe_memory == null) {
maybe_memory = new WebAssembly.Memory({initial:17,maximum:16384,shared:true});
}
memory = imports.wbg.memory = maybe_memory;
if (typeof Response === 'function' && module instanceof Response) {
if (typeof WebAssembly.instantiateStreaming === 'function') {
try {
return await WebAssembly.instantiateStreaming(module, imports);
} catch (e) {
if (module.headers.get('Content-Type') != 'application/wasm') {
console.warn("`WebAssembly.instantiateStreaming` failed because your server does not serve wasm with `application/wasm` MIME type. Falling back to `WebAssembly.instantiate` which is slower. Original error:\n", e);
} else {
throw e;
}
}
}
const bytes = await module.arrayBuffer();
return await WebAssembly.instantiate(bytes, imports);
} else {
const instance = await WebAssembly.instantiate(module, imports);
if (instance instanceof WebAssembly.Instance) {
return { instance, module };
} else {
return instance;
}
}
} Also the export default function init (module_or_path?: InitInput | Promise<InitInput>, maybe_memory?: WebAssembly.Memory): Promise<InitOutput>; |
Ah sorry, yes, that seems like a reasonable update to me! |
Can confirm, we discussed this on the original PR and I've also ran into this recently and left a comment there but haven't gotten around to submitting issue. #1412 (comment) Thanks @d3lm! |
Yep, I agree with @Pauan. The code he provided is what I was hoping for, to make the wasm memory optional especially for the generated type definition. |
As the name implies, it's already optional, but wasn't marked as such in TS. We could put some more complicated / stricter types here depending on type of the first argument, but at least this fixes the issue for TS consumers. Fixes rustwasm#2133
* Make maybe_memory optional in TS As the name implies, it's already optional, but wasn't marked as such in TS. We could put some more complicated / stricter types here depending on type of the first argument, but at least this fixes the issue for TS consumers. Fixes #2133 * Add rust-toolchain to raytrace example It should always be built with nightly, and this file sets the toolchain for Rustup. * Rework init_memory - Unify `init_memory` for `maybe_memory` case to use either the explicitly given value or the default (`new WebAssembly.Memory(...)`). - Move it to the main `init` function where all other `imports` are assigned too. - Remove global `memory` variable which doesn't seem to be used by anything. * Format * Update cargo fmt & reformat again * Use explicit nightly version for Raytracer on CI * Delete rust-toolchain * Update azure-pipelines.yml
Describe the Bug
I have noticed that when the compilation target is
web
that themaybe_parameter
of theiniti(input, maybe_memory)
is required. The name implies that it might be missing and especially for the first call toinit
it's most likely missing ifinput
is a string and the module is fetched. Only subsequent calls might have memory. Maybe we change the type of this to also acceptnull
orundefined
or mark it as optional.I think it would make sense to have a sanity check in
load
that, ifmodule
is not a function nor aResponse
object, meaning it's a WebAssembly module, then we add a check to ensure thatmaybe_memory
is notundefined
.Alternatively, we can pass
null!
formemory_memory
but to me this feels hacky.Steps to Reproduce
wasm32-unknown-unknown
with target beingweb
If there is a solution to this and a clear path, I am volunteering to work on a PR to get this resolved.
The text was updated successfully, but these errors were encountered: