-
-
Notifications
You must be signed in to change notification settings - Fork 593
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
feat(wasm): allow using a custom loader #1686
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,23 @@ import type { FilterPattern } from '@rollup/pluginutils'; | |
*/ | ||
export type TargetEnv = 'auto' | 'auto-inline' | 'browser' | 'node'; | ||
|
||
/** | ||
* The type for the plugin's loader function | ||
* | ||
* This is the function that ends up called when encountering a WASM import to load it and turn it into an usable object at runtime. | ||
* | ||
* @param {boolean} sync Whether the load should happen synchronously or not | ||
* @param {string | null} filepath The path to the module. | ||
* @param {string | null} src The base64-encoded source of the module | ||
* @param {any} imports An object containing the module's imports | ||
*/ | ||
export type WasmLoaderFunction = ( | ||
sync: boolean, | ||
filepath: string | null, | ||
src: string, | ||
imports: any | ||
) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This return type should be something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a while, so I forgot a little about how it works, but as I could see, the loader function's return value isn't used anywhere? The pre-change loading code was this: `import { _loadWasmModule } from ${JSON.stringify(HELPERS_ID)};
export default function(imports){return _loadWasmModule(${+isSync}, ${publicFilepath}, ${src}, imports)}` but as far as I can see, that return could just as well not be here, since I don't see how you could make use of it and there's no higher-level code expecting anything out of it… There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The code importing the wasm module expects the return value. If the {
loader: async (sync, filepath, src, imports) => {
const fs = require("fs/promises");
const path = require("path");
const wasmBuffer = await fs.readFile(path.resolve(__dirname, filepath));
return WebAssembly.compile(wasmBuffer);
},
} While it is possible to write a wasm library with a loader that assigns to a global variable instead of use the return value, I don't think that would be recommended. |
||
|
||
export interface RollupWasmOptions { | ||
/** | ||
* A picomatch pattern, or array of patterns, which specifies the files in the build the plugin | ||
|
@@ -40,8 +57,22 @@ export interface RollupWasmOptions { | |
* A string which will be added in front of filenames when they are not inlined but are copied. | ||
*/ | ||
publicPath?: string; | ||
/** | ||
* The loader used to process WASM modules. | ||
* | ||
* This plugin provides 4 default loaders: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "builtin" might be a better word than default, as I can see it being useful to call out Btw, the readme probably should be updated in this PR too, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. builtin indeed sounds better. I'll switch to that. Will go over the readme once the changes are okay-ed |
||
* - `"auto"` will determine the environment at runtime and invoke the correct methods accordingly | ||
* - `"auto-inline"` always inlines the Wasm and will decode it according to the environment | ||
* - `"browser"` omits emitting code that requires node.js builtin modules that may play havoc on downstream bundlers | ||
* - `"node"` omits emitting code that requires `fetch` | ||
* | ||
* Additionally, you can pass your own loader function if you need better control. The plugin expects a | ||
* function with the following signature: `_loadWasmModule(sync: boolean, filepath: string, src: string, imports: any)`. | ||
*/ | ||
loader?: TargetEnv | WasmLoaderFunction; | ||
/** | ||
* Configures what code is emitted to instantiate the Wasm (both inline and separate) | ||
* @deprecated Use {@link RollupWasmOptions.loader} | ||
*/ | ||
targetEnv?: TargetEnv; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a type signature that we (as in rollup) are willing to commit to exposing? Previously it was internal and I'm assuming not much thought was given. I'm flagging this in case there is a concern about future compatibility issues that arise from the need to update, reorder, or remove parameters that could impact all parameters -- not just the ones that are changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly? I don't feel confident on that part. It might be possible to hide this type externally, but IMO I'd prefer to be sure we agree on the way forward w.r.t the loader as a function/name issue first.