Skip to content

Enable crunching with X-Crunch header and crunch entire response#1061

Merged
alloy merged 5 commits intoartsy:masterfrom
just-be-dev:add_crunch_header
May 30, 2018
Merged

Enable crunching with X-Crunch header and crunch entire response#1061
alloy merged 5 commits intoartsy:masterfrom
just-be-dev:add_crunch_header

Conversation

@just-be-dev
Copy link
Copy Markdown
Contributor

@just-be-dev just-be-dev commented May 28, 2018

This allows enabling the crunching of a response (#1042) via an X-Crunch header. It'll just be a little bit easier to integrate into some of the projects this way.

In emission, the current approach would require us to process the URL to safely add the query parameter. Enabling it via a header would just be a bit cleaner.

This also updates how crunching works. It crunches the entire response instead of just the data portion. Being as crunching is a lossless process there's no harm in doing so. Also it reduces the overall logic that we have to share across projects.

@just-be-dev just-be-dev changed the title Enable crunching with X-Crunch header Enable crunching with X-Crunch header and crunch entire response May 28, 2018
@just-be-dev
Copy link
Copy Markdown
Contributor Author

See artsy/emission#1068 for emission implementation.

@just-be-dev
Copy link
Copy Markdown
Contributor Author

When responses are crunched the response now has an X-Crunch header as well. This header gives the clients something to hook onto to know if the incoming response should be uncrunched.

@alloy
Copy link
Copy Markdown
Contributor

alloy commented May 30, 2018

Yup, this a good idea 👍

As an aside (no blocker), do we even really need the query parameter?

@alloy alloy merged commit 1205da8 into artsy:master May 30, 2018
@just-be-dev
Copy link
Copy Markdown
Contributor Author

It's not strictly necessary, but it also doesn't really hurt. It's probably easier to use graphiql with the query param. It's just another options for any clients that are connecting.

@just-be-dev just-be-dev deleted the add_crunch_header branch May 30, 2018 21:43
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