Skip to content
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

Update brotli version and add dictionary compression encoding #51

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

yoavweiss
Copy link
Contributor

This PR updates the brotli code to the latest version, updates the Makefile to be able to build that, and moves to use brotli's streaming interface, instead of the older encode() interface.

In order to do that latter, I've added compress.c, which calls that streaming interface, and which includes dictionary as its inputs. When a dictionary is provided, it's attached to the Brotli state and used as part of the compression.

This PR does not add the equivalent dictionary capabilities to the decoder. A reasonable followup may be to call the WASM decoder for that purpose.

@yoavweiss yoavweiss changed the title Update brotli to the latest version and add the ability to compress with a dictionary Update brotli version and add dictionary compression encoding Feb 27, 2024
ENCSRC = enc/encode.c $(wildcard $(COMMONDIR)/*.c) $(wildcard $(ENCDIR)/*.c)
COMMONDIR = vendor/brotli/c/common
ENCDIR = vendor/brotli/c/enc
ENCSRC = compress.c enc/encode.c $(wildcard $(COMMONDIR)/*.c) $(wildcard $(ENCDIR)/*.c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess we don't need encode.c anymore since the js wrapper always uses encodeWithDictionary?

@devongovett devongovett merged commit 577e453 into foliojs:master Feb 28, 2024
@devongovett
Copy link
Member

Thanks!

@devongovett
Copy link
Member

one problem with the change to compile to wasm is that bundlers have a hard time picking up where the wasm file is located. emscripten doesn't produce very statically analyzable output. could change emscripten to output ES modules so it can use import.meta.url but that might be a breaking change (it makes everything async). will need to consider what to do here before publishing.

@yoavweiss
Copy link
Contributor Author

one problem with the change to compile to wasm is that bundlers have a hard time picking up where the wasm file is located.

Apologies, but I'm not sure I get the implications of that :)
Is this something that my change introduced?

@surma
Copy link

surma commented Feb 28, 2024

I think switching to ESM output is generally a good idea. You could (should?) expose an option where you can pass the path/URL to the wasm blob, which usually is enough to let developers use a wasm-based library with any bundler tool.

@jakearchibald
Copy link

jakearchibald commented Feb 28, 2024

You could (should?) expose an option where you can pass the path/URL to the wasm blob, which usually is enough to let developers use a wasm-based library with any bundler tool.

Coincidentally, I was playing with https://github.com/httptoolkit/brotli-wasm this morning and the lack of this was a blocker when using a Vite project.

Here's their entry point: https://www.npmjs.com/package/brotli-wasm?activeTab=code

import init, * as brotliWasm from "./pkg.web/brotli_wasm";
export default init().then(() => brotliWasm);

Annoyingly, the init function (generated by wasm-bingen) does accept a URL. I patched it so the entry point was:

import wasmURL from './pkg.web/brotli_wasm_bg.wasm';
import init, { compress } from './pkg.web/brotli_wasm';
await init(wasmURL);
export { compress };

And that worked great.

So, yeah, +1 to allowing the wasm URL to be passed in, and ensure your wasm is exported in package.json.

@devongovett
Copy link
Member

Is this something that my change introduced?

Yeah but only due to the upgrade of Emscripten. The previous one was so old it was still generating asm.js. That meant it was a single JS file. Now it needs to reference a WASM file as well.

Moving to ESM and making the WASM loading async would need to be done in a major version bump.

yoavweiss added a commit to yoavweiss/brotli.js that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants