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

Add the Accept-Encoding header to the cache key #1

Merged
merged 2 commits into from
Apr 12, 2016
Merged

Add the Accept-Encoding header to the cache key #1

merged 2 commits into from
Apr 12, 2016

Conversation

trasch
Copy link
Contributor

@trasch trasch commented Apr 11, 2016

This is necessary so that clients who request an uncompressed body won't get a gzip'ed result (and vice versa, but that would be less critical).

Note: There might be more headers to check for, because they might change the result body.

@codecov-io
Copy link

Current coverage is 100.00%

Merging #1 into master will not affect coverage as of 683e06b

@@            master      #1   diff @@
======================================
  Files            1       1       
  Stmts           21      21       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit             21      21       
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 683e06b

Powered by Codecov. Updated on successful CI builds.

@trasch trasch closed this Apr 11, 2016
@trasch trasch deleted the patch-1 branch April 11, 2016 15:33
@trasch trasch restored the patch-1 branch April 11, 2016 15:43
@trasch trasch reopened this Apr 11, 2016
@brianreavis
Copy link
Member

I worry that clients' Accept-Encoding can be pretty varied, which will bloat the cache. What do you think of making they cache key configurable like below?

lru({key: function(req) {
    return req.z+','+req.x+','+req.y+','+req.layer+','+req.filename+','+req.headers['accept-encoding'];
}});

This way we keep the default behavior for most cases where it doesn't make a difference – yet there's a way to deal with cases like you mention. It's a little clunky but is most configurable. With this sort of setup you could even cut down on cache space like so:

lru({key: function(req) {
    var compressed = /gzip|deflate/.test(req.headers['accept-encoding']) ? 1 : 0;
    return req.z+','+req.x+','+req.y+','+req.layer+','+req.filename+','+compressed;
}});

@trasch
Copy link
Contributor Author

trasch commented Apr 12, 2016

Good idea, I changed the patch accordingly!

@brianreavis brianreavis merged commit 4482fbe into naturalatlas:master Apr 12, 2016
@brianreavis
Copy link
Member

Awesome! This is now out: 0.2.0. Really appreciate all the misc fixes / improvements you've been putting in.

@trasch
Copy link
Contributor Author

trasch commented Apr 12, 2016

You're welcome, and I appreciate all the effort you've already put into Tilestrata! :-)

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.

3 participants