Add Brotli WASM module imports to package.json #40
Conversation
|
|
|
Nice work, thanks @oligamiq! I'm away this week I'm afraid but I'll have a proper look at this and test everything next week and get back to you. |
oligamiq
force-pushed
the
main
branch
2 times, most recently
from
084b2fd to
6f7a6fd
Compare
package.json
Outdated
| "./web/bg.wasm": { | ||
| "types": "./pkg/brotli_wasm_bg.wasm.d.ts", | ||
| "import": "./pkg/brotli_wasm_bg.wasm", | ||
| "default": "./pkg/brotli_wasm_bg.wasm" |
There was a problem hiding this comment.
This doesn't work as /pkg/ doesn't exist.
package.json
Outdated
| @@ -12,6 +12,16 @@ | |||
| "browser": "./index.browser.js", | |||
| "require": "./index.node.js", | |||
| "default": "./index.web.js" | |||
| }, | |||
| "./web": { | |||
There was a problem hiding this comment.
Can you explain more about why we need a separate /web path? The brotli_wasm.js file is already imported in index.web.js - can't we just export the methods directly from there? That would also allow us to also provide a single API to do this that worked for all targets.
I do agree there's improvements to make here, but I'm not sure that just adding more exports is correct. It really seems like we should be able to provide a small set of exports, correctly automatically select the right one for the environment, and export data in the formats required.
If that needs to be "JS export + raw WASM" export for environments that need to manage wasm import & init manually, that makes sense, but I'm not clear why we'd need "JS export + web JS export + web raw WASM export", which seems like what we're doing here.
|
Will this be merged soon? |
|
@TimTheBig see my comments above, this isn't mergable at the moment. If @oligamiq has time to resolve those or if you want to open a new PR for this, I'd be happy to review and keep this moving forward. |
The init function is provided solely for the web target, and if one were to tack it-along with its type-onto the JS exports, it would simply become noise for everyone else. As for importing the WASM asset, we may choose either of these: import wasm from '../node_modules/brotli-wasm/pkg.web/brotli_wasm_bg.wasm'
import wasm from 'brotli-wasm/web/bg.wasm'but for the sake of elegance, I’ve enabled the latter. |
|
For me the wasm does not work with vite as it does not get packed |
It should be able to be written like this. https://github.com/oligamiq/brotli-wasm/tree/raw-wasm/example/test-raw-wasm |
|
Hi @pimterry, would you mind taking a look at this PR when you have a moment? |
| "default": { | ||
| "types": "./pkg.web/brotli_wasm_bg.wasm.d.ts", | ||
| "default": "./pkg.web/brotli_wasm_bg.wasm" | ||
| } |
There was a problem hiding this comment.
This seems fine now, and makes perfect sense, and /wasm is a good pattern imo 👍 Very happy to ship this.
| * @returns {Promise<InitOutput>} | ||
| */ | ||
| export function init (module_or_path?: InitInput | Promise<InitInput>): Promise<InitOutput | void>; |
There was a problem hiding this comment.
This API is definitely odd though. I don't think having a stub function that doesn't work (or even return something useful) is going to be nice going forwards. The goal is that you can write the same code and it'll work everywhere, and if init() returns different things in different environments then that's not true.
Can we make the below type work?
export function init(module_or_path?: InitInput | Promise<InitInput>): Promise<BrotliWasmType>;I'm imagining that would be possible by making every module do either:
export async function init(input) {
await brotliWasmInit(input);
return brotliWasm;
}or
export async function init() {
return brotliWasm;
}That way we can expose an init function that works the same way everywhere, but does call the custom init behaviour on the web case where required. Would that cover your use case? It's quite possible I'm missing something (do you need access to the InitOutput data itself? Which part and why?) I'm just trying to explore consistent APIs to solve this for everybody in the same way instead of fragmenting by environment.
#31