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

support zstd formatted responses #364

Merged
merged 2 commits into from May 18, 2024
Merged

support zstd formatted responses #364

merged 2 commits into from May 18, 2024

Conversation

zuisong
Copy link
Contributor

@zuisong zuisong commented Apr 13, 2024

Chrome versions after 123 already support zstd. (can i use zstd)
Firefox will support zstd mozilla/standards-positions#775 (comment)

Maybe we should also support zstd formatted responses.

image
  • curl support zstd response
     ❯ curl --header 'accept-encoding:zstd'  --compressed https://www.facebook.com/data/manifest/
     {"gcm_sender_id":"15057814354","gcm_user_visible_only":true,"edge_side_panel":{"preferred_width":376},"short_name":"Facebook","name":"Facebook","start_url":"\/?ref=homescreenpwa","display":"minimal-ui","background_color":"#FFFFFF","theme_color":"#1877F2","icons":[{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/y0\/r\/eFZD1KABzRA.png","sizes":"192x192","type":"image\/png"},{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/yd\/r\/DeNyZD1Vj3q.png","sizes":"512x512","type":"image\/png"}],"widgets":[{"name":"Facebook","description":"Facebook","short_name":"Facebook","tag":"fb_widget","type":"application\/json","update":100,"icons":[{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/y0\/r\/eFZD1KABzRA.png","sizes":"192x192"}],"screenshots":[{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/yT\/r\/mqlhqxHpT-X.png","sizes":"464x478","label":"Widget Screenshot"}],"template":"dummy","data":"\/dummy.json","ms_ac_template":"\/dummy.json"}],"related_applications":[{"platform":"play","id":"com.facebook.katana"},{"platform":"play","id":"com.facebook.lite"},{"platform":"play","id":"com.facebook.orca"},{"platform":"play","id":"com.facebook.mlite"}],"prefer_related_applications":false}⏎                                                                 
    

  • xh current
     ❯ xh https://www.facebook.com/data/manifest/ accept-encoding:zstd -b
     +-----------------------------------------+
     | NOTE: binary data not shown in terminal |
     +-----------------------------------------+
    
  • after
     ❯ cargo run -- https://www.facebook.com/data/manifest/ accept-encoding:zstd -b --pretty=none
         Finished dev [unoptimized + debuginfo] target(s) in 0.08s
          Running `/Users/chen/.rust-target/debug/xh 'https://www.facebook.com/data/manifest/' 'accept-encoding:zstd' -b --pretty=none`
     {"gcm_sender_id":"15057814354","gcm_user_visible_only":true,"edge_side_panel":{"preferred_width":376},"short_name":"Facebook","name":"Facebook","start_url":"\/?ref=homescreenpwa","display":"minimal-ui","background_color":"#FFFFFF","theme_color":"#1877F2","icons":[{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/y0\/r\/eFZD1KABzRA.png","sizes":"192x192","type":"image\/png"},{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/yd\/r\/DeNyZD1Vj3q.png","sizes":"512x512","type":"image\/png"}],"widgets":[{"name":"Facebook","description":"Facebook","short_name":"Facebook","tag":"fb_widget","type":"application\/json","update":100,"icons":[{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/y0\/r\/eFZD1KABzRA.png","sizes":"192x192"}],"screenshots":[{"src":"https:\/\/static.xx.fbcdn.net\/rsrc.php\/v3\/yT\/r\/mqlhqxHpT-X.png","sizes":"464x478","label":"Widget Screenshot"}],"template":"dummy","data":"\/dummy.json","ms_ac_template":"\/dummy.json"}],"related_applications":[{"platform":"play","id":"com.facebook.katana"},{"platform":"play","id":"com.facebook.lite"},{"platform":"play","id":"com.facebook.orca"},{"platform":"play","id":"com.facebook.mlite"}],"prefer_related_applications":false}
    

@zuisong zuisong changed the title decode responses in zstd format support zstd formatted responses Apr 13, 2024
@blyxxyz blyxxyz self-requested a review April 16, 2024 19:33
@blyxxyz
Copy link
Collaborator

blyxxyz commented Apr 16, 2024

I'm unsure about this. It'd add another 150KB to the binary (pushing us over 8MB). And I don't know how much upside there is. In which cases would somebody really need it or be helped by it? (In download mode we don't even enable compression.)

I'm also worried about whether this can dynamically link to the system's libzstd, since that's how (some) distros prefer to work. openssl-sys has some cargo features related to this but I don't see that in zstd-sys. I don't really know the details about how this is normally handled and what -sys packages tend to do but it might be a headache (or just busywork) for distro maintainers.

(The ruzstd crate could potentially help with both of these but I don't know how mature it is.)

The code does look good.

@zuisong zuisong marked this pull request as ready for review May 7, 2024 08:03
Copy link
Owner

@ducaale ducaale left a comment

Choose a reason for hiding this comment

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

Looks good!

@blyxxyz is there anything to change before we merge this PR?

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks like the right way to do it. I'm still a little worried about the binary size but it is nice to keep up with browsers.

@ducaale ducaale merged commit f3c05e0 into ducaale:master May 18, 2024
9 checks passed
@zuisong zuisong deleted the zstd branch May 19, 2024 01:52
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

3 participants