-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: upload br encoding to asset canister #3791
feat: upload br encoding to asset canister #3791
Conversation
@@ -370,7 +370,9 @@ fn content_encoding_descriptive_suffix(content_encoding: &str) -> String { | |||
// todo: make this configurable https://github.com/dfinity/dx-triage/issues/152 | |||
fn applicable_encoders(media_type: &Mime) -> Vec<ContentEncoder> { | |||
match (media_type.type_(), media_type.subtype()) { | |||
(mime::TEXT, _) | (_, mime::JAVASCRIPT) | (_, mime::HTML) => vec![ContentEncoder::Gzip], | |||
(mime::TEXT, _) | (_, mime::JAVASCRIPT) | (_, mime::HTML) => { | |||
vec![ContentEncoder::Gzip, ContentEncoder::Brotli] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been discussed, as a default behavior, at the product level?
I think it would be better to opt-in, configured in .ic-assets.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sns-controlled asset canisters might impossible to sync unless they can opt-in for a subset of files rather than all at once. This is because a proposal's commit by definition has to be a single call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be adjusted once #3792 is merged
Description
Asset synchronization now not only uploads
identity
andgzip
as encodings for MIME typestext
,js
, andhtml
, but alsobr
encoding.Fixes SDK-1604
How Has This Been Tested?
Added e2e
Checklist: