cnnlabs / cnn-hapi

CNN Hapi
10 stars 4 forks source link

Cache header issues #39

Closed ghost closed 7 years ago

ghost commented 8 years ago

Two kinda related things, I wanted to get a second opinion before working on a resolution.

Example app suggests setting maxAge: 10, which results in header: Cache-Control: 10

If the api is maxAge: N, it should populate to Cache-Control: max-age=N (rather than Cache-Control: max-age=10).

Alternately there is a good argument to be made for ditching maxAge and adopting the key/value format used by Surrogate-Cache-Control: surrogateCacheControl: 'max-age=60, stale-while-revalidate=10, stale-if-error=6400'.

onPreResponse cache control defaults overwrite cache settings provided by routes

The server-wide cache settings in the onPreResponse hook override headers set by individual routes:

app.route({
    method: 'GET', path: '/',
    handler: function routeHandler(request, reply) {
        reply('success')
            .header('Cache-Control', 'private, max-age=42'); // <- gets overridden
    }
});

I believe directives in the route should override site-wide defaults. Yes?

Additionally, I think this strengthens the argument for removing maxAge and using more flexible Cache-Control default ... I suspect the solution to the above would be to look for any sort of custom header and not apply the default header if there is a header set by application logic in a route.

So, in my opinion, choices are:

adslaton commented 8 years ago

This is a good idea.

ghost commented 8 years ago

Hahaha, which? Personally I'd favor:

adslaton commented 8 years ago

I think we ditch maxAge, offer to users a cacheControl string to set in arguments/environment, allow override in routes