httptoolkit / brotli-wasm

A reliable compressor and decompressor for Brotli, supporting node & browsers via wasm
Apache License 2.0
264 stars 21 forks source link

Create web version of build for vite and rollup, bump versions of libraries #13

Closed jonluca closed 2 years ago

jonluca commented 2 years ago

This fixes https://github.com/httptoolkit/brotli-wasm/issues/8

This adds a third build package with the async import for browsers that is compatible with vite and rollup. You'll need to change your imports and add a vite plugin for it to work, illustrated below. No other changes, and all existing code works as is.

Where you want to use your code:

import init, { decompress } from "brotli-wasm/pkg.web/brotli_wasm";

const initPromise = init("brotli_wasm_bg.wasm");
export const brotliDecompress = zlib.brotliDecompress
  ? promisify(zlib.brotliDecompress)
  : async (buffer: Uint8Array): Promise<Uint8Array | undefined> => {
      try {
        await initPromise;
        const output = decompress(buffer);
        return output;
      } catch (e) {
        console.error(e);
        return;
      }
    };

And have your vite config be

import { defineConfig } from "vite";
import wasm from "vite-plugin-wasm";
import { wasm as rollupWasm } from "@rollup/plugin-wasm";
import { viteStaticCopy } from "vite-plugin-static-copy";

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [
    viteStaticCopy({
      targets: [
        {
          src: require.resolve("brotli-wasm/pkg.web/brotli_wasm_bg.wasm"),
          dest: ".",
        },
      ],
    }),
    wasm(),
    topLevelAwait(),
  ],
  build: {
    minify: false,
    target: ["esnext"],
    sourcemap: true,
    rollupOptions: {
      treeshake: false,
      plugins: [rollupWasm()],
    },
    ssr: false,
  },
});
jonluca commented 2 years ago

Mocha v10 doesn't support node 10, which is why the tests were failing. Downgraded it and confirmed tests work on local version of node 10.

pimterry commented 2 years ago

Ok - why are we updating to Mocha 10 here though?

jonluca commented 2 years ago

I was just updating all the dependencies - there are some pretty nice performance gains to be had in the latest versions of mocha

I actually tried to move it to vitest (you should give it a shot, it's so much faster), but vite still isn't great with wasm (hence the PR).

pimterry commented 2 years ago

It's useful to do updates like that, sure, and I'm definitely open to bumping Mocha and dropping Node 10 entirely to be honest, but let's keep that separate to this change unless it's directly necessary please.

It makes it much much easier to debug future issues, revert problematic changes, and understand which code is related to which features if unrelated changes aren't batched together. Makes git blame & git bisect a lot more effective :smile:

Are any of the cargo & package.json dependency updates here required to make this work? If not, let's drop them. Happy to do some nice-to-have dependency updates later of course, but only once we've got this working properly by itself.

Any thoughts on the testing setup for this? I imagine we'd want to run Vite tests in a real browser to make them representative. We could probably make a second karma config to run the same test script but replacing the webpack config with whatever's required to do the Vite setup here. I guess Vitest is preferable for Vite though. Is there an obvious way to define one test suite that can be run within pure node, via webpack in a browser, and via Vite?

jonluca commented 2 years ago

That makes sense. I've removed the version changes besides the ones that were absolutely necessary.

I'll make another PR to drop node 10 support, bump the versions, and also build release outputs of wasm.

Testing is interesting - I've changed the tests to test the pkg.web version. Vite is a bundler, so it doesn't make that much sense to test a vite specific suite. Testing the different wasm-pack output does make sense though. Let me know if you can think of a cleaner way of doing the tests than what I did, by the way, since it's a bit odd with the i === 0 test.

jonluca commented 2 years ago

https://github.com/jonluca/brotli-wasm/tree/feat/modernize this branch will drop support for node 10 FYI

pimterry commented 2 years ago

Hi @jonluca, I did a whole bunch of digging into this, and I think I've found some good options to bring this all together and simplify it.

I've put together a POC to test that out, on top of your changes here. That looks like this: https://github.com/httptoolkit/brotli-wasm/commit/vite-support

That commit:

I think this all works - the three test suites pass, and I've tested it manually elsewhere, and it's still backward compatible with both my existing Node & Webpack usage, and the new Vite tests here seem to compile and work fine AFAICT.

The problem is that I don't really understand Vite though :smile:. You mentioned that you need to pass the path to the WASM file to init() in some cases. Which cases? In the test suite here everything compiles and handles that automatically, or so it seems. This is using karma-vite, which is just running vite 2.9.14 with default config internally AFAICT.

Is there something that's not representative in that test setup?

When you have a minute, can you try checking out that branch, building it locally, and then using this directly in your Vite code, with no special WASM setup? I think it should work like this:

import brotliPromise from 'brotli-wasm';
const brotli = await brotliPromise;

brotli.decompress(...) // etc

If there is a clear special case where you need to pass the path, we can document that and add an extra API to support just that use case, but it seems like at least some of the time that's not necessary, and it'd be great to avoid it if we can.

jonluca commented 2 years ago

This looks good.

You mentioned that you need to pass the path to the WASM file to init() in some cases. Which cases? In the test suite here everything compiles and handles that automatically, or so it seems. This is using karma-vite, which is just running vite 2.9.14 with default config internally AFAICT.

Your index.web.js might be a better solution. I just tested it on a clean vite setup and it seems to be working. The issue I was having was that vite build uses rollup. Init takes in the configuration to load the wasm, which is one of RequestInfo | URL | Response | BufferSource | WebAssembly.Module; If it isn't passed, it will try and load it via url.

Rollup was trying to fetch the brotli_wasm_bg.wasm file from /, because it was calling init() with no parameters. The only solution was to copy that asset over, but if it's within the module itself it might know to inline it/rename it properly.

This looks great though.

pimterry commented 2 years ago

Great! Thanks for testing that.

Your points about init() make sense, and I've played around a little with adding a separate initialization method in the API, but I think since we'd need to expose a promise that always calls init() regardless for this approach, it'd be a bit messy to try to add a separate API for manually setting the path. For now I think it's OK to just go forward with this approach, but if you do find a clear use case you can reproduce where this breaks, do please share a repro and I'll look more into ways to handle that better. At the very least, this is a good start that should solve some real problems in the meantime.

For now, I've tidied that commit up a bit, merged it with your changes here and then merged the lot :smile:. Thanks for contributing this! Glad we got to a good working result here. I'll do a release to publish all this to NPM in just a minute.

jonluca commented 2 years ago

Yes this is great. Thanks for cleaning it up. Agreed!