Add option to override locateFile for loading WASM files from custom sources#53
Add option to override locateFile for loading WASM files from custom sources#53ronkorland wants to merge 5 commits into
Conversation
…sources (e.g., local path or remote URL)
moshen
left a comment
moshen
left a comment
There was a problem hiding this comment.
This is a great idea, and would certainly make the browser use-case for the module easier. However as it appears, I'm not exactly sure it would work as implemented.
Also please add a description of the option to the README.
| loadDefaultMagicfile?: boolean; | ||
| magicFiles?: Uint8Array[]; | ||
| stdio?: StdioOverrideFunction; | ||
| locateFile?: () => string, |
There was a problem hiding this comment.
Should be changed to an explicit function type signature, or simply accepting a string and providing a function below.
There was a problem hiding this comment.
I update the README + update the function signature to locateFile?: (path: string, prefix: string) => string
| ): Promise<WASMagic> { | ||
| const Module = await libmagicFactory(); | ||
| const Module = await libmagicFactory({ | ||
| locateFile: options.locateFile, |
There was a problem hiding this comment.
locateFile appears to expect a function:
https://emscripten.org/docs/api_reference/module.html#Module.locateFile
There was a problem hiding this comment.
locateFile is define as function in the WASMagicOptions type
I update the README file, please validate again |
|
@moshen can you please take a look at my latest changes? |
|
@ronkorland Will do. Had to fix publishing with the recent npm changes. |
@moshen do you think you can release/merge this PR this week? |
|
@moshen I’d really like to use this change is there anything I can do to help move this PR forward or get it merged and released? |
|
Sorry I haven't gotten back to you. It's been a busy month unfortunately. I'm circling back to this now and was testing this out locally. I was going to flatten the property into the larger config object. Any objections? |
|
@ronkorland this is what I was thinking: #55 |
|
I start with flatten object but I think it not I saw their is many properties that we can pass to the |
|
My initial reaction is that there currently aren't any that are needed on Module, but several that would break the usage of this library. Accepting an object in Javascript and passing it whole into the constructor means potential issues (intentionally or unintentionally) that I'm not keen on supporting. I'm much more of a fan of being explicit in what's accepted than implicit here. Since it sounds like you're using this on the frontend, you might want |
|
@moshen ok, lets continue with your PR |
|
If it needs more options exposed can do that later. I'm of two minds on it. Either expose everything from |
- Add option to override locateFile for loading WASM files from custom sources (e.g., local path or remote URL) Co-authored-by: Ron Korland <[email protected]>
|
Closed by merging #55 |
2 participants
Adds support for overriding the locateFile function to control how the WASM file is loaded (e.g., from local path, CDN, or Blob URL).
Backward-compatible and doesn’t change existing behavior.