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

Make WOFF2 brotli decoding optional #110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fschutt
Copy link
Contributor

@fschutt fschutt commented Feb 3, 2025

The brotli-decompressor crate has some C dependencies, which is a problem when compiling allsorts to WASM (in order to use in on the web and on serverless-wasm applications). Specifically, some tools have problems with mixed C / Rust dependencies.

This PR makes --feature=brotli optional, but enabled by default.

@wezm
Copy link
Contributor

wezm commented Feb 4, 2025

I'm ok with the overall goal of the PR. However doing some review and testing I'm wondering if this should be a feature to enable/disable WOFF2 support instead of just brotli. It seems that it's not possible to use the ReadBinary impl for Woff2Font without the brotli feature enabled, which in turn makes it unlikely you're going to be able to call the extended_metadata method, which will also fail if the brotli feature is disabled.

Woff2Font being the primary interface to WOFF2 fonts I'm leaning towards a woff2 cargo feature that enables/disables the whole woff2 module. That way there isn't the possibility of building code that will only fail at runtime. For example, if someone used no-default-features and did not add brotli back they could write code that would compile, but then fail when used to read a WOFF2 font. In the alternative where the woff2 module is conditional on the woff2 feature the code will fail to build if the feature was absent.

Does that sound like a reasonable alternative?

@fschutt
Copy link
Contributor Author

fschutt commented Feb 4, 2025

Yeah ok, the reason I did it like this is because I didn't want to break the API.

@wezm
Copy link
Contributor

wezm commented Feb 6, 2025

Did you want to tackle that change or are you wanting me to do it?

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.

2 participants