Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Commit

Permalink
use etagify for content-based hashes in ETag headers - issue #1331
Browse files Browse the repository at this point in the history
  • Loading branch information
@lloyd
lloyd committed Mar 26, 2012
1 parent 1971ecc commit edd34eb
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 22 deletions.
4 changes: 2 additions & 2 deletions config/local.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"driver": "json"
},
"express_log_format": "dev_bid",
"email_to_console": true
"email_to_console": true,
"env": "local"
}

36 changes: 16 additions & 20 deletions lib/browserid/views.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,42 +11,38 @@ connect = require('connect'),
config = require('../configuration.js'),
und = require('underscore'),
util = require('util'),
httputils = require('../httputils.js');
httputils = require('../httputils.js'),
etagify = require('etagify');

// all templated content, redirects, and renames are handled here.
// anything that is not an api, and not static
const
path = require('path');


const VIEW_PATH = path.join(__dirname, "..", "..", "resources", "views");

// none of our views include dynamic data. all of them should be served
// with reasonable cache headers. This wrapper around rendering handles
// cache headers maximally leveraging the same logic that connect uses
// issue #910
function renderCachableView(req, res, template, options) {
fs.stat(path.join(VIEW_PATH, template), function (err, stat) {
res.setHeader('Date', new Date().toUTCString());
res.setHeader('Vary', 'Accept-Encoding,Accept-Language');
if (config.get('env') !== 'local') {
// allow caching, but require revalidation via ETag
res.setHeader('Cache-Control', 'public, max-age=0');
res.setHeader('ETag', util.format('"%s-%s-%s"', stat.size, Number(stat.mtime), req.lang));
} else {
res.setHeader('Cache-Control', 'no-store');
}
res.setHeader('Content-Type', 'text/html; charset=utf8');
if (connect.utils.conditionalGET(req)) {
if (!connect.utils.modified(req, res)) {
return connect.utils.notModified(res);
}
}
res.render(template, options);
});
if (config.get('env') !== 'local') {
// allow caching, but require revalidation via ETag
res.etagify();

This comment has been minimized.

Sign in to view

Copy link
@ozten

ozten Mar 26, 2012

Contributor

Very cool. I like how simple this is. Won't be bad if it's sprinkled through routes in an app that doesn't have a single point like ours (renderCachableView).

This comment has been minimized.

Sign in to view

Copy link
@lloyd

lloyd Mar 27, 2012

Author Contributor

cool, I'm glad you like it!

The magic that makes it work is that the first time the resource is served, a bunch of work is done setting up proxies to capture the outbound bytes - but the second time we never even get into the handler in the case if-none-match contains up-to-date sha, and if it doesn't then the .etagify() is basically a no-op.
res.setHeader('Cache-Control', 'public, max-age=0');
} else {
// disable all caching for local dev
res.setHeader('Cache-Control', 'no-store');
}
res.setHeader('Date', new Date().toUTCString());
res.setHeader('Vary', 'Accept-Encoding,Accept-Language');
res.setHeader('Content-Type', 'text/html; charset=utf8');
res.render(template, options);
}

exports.setup = function(app) {
app.use(etagify());

app.set("views", VIEW_PATH);

app.set('view options', {
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"connect-cookie-session": "0.0.2",
"connect-logger-statsd": "0.0.1",
"ejs": "0.4.3",
"etagify": "0.0.1",

This comment has been minimized.

Sign in to view

Copy link
@ozten

ozten Mar 26, 2012

Contributor

Couple meta comments:

  • This is very sympathetic to mozilla/connect-cachify - keep as different modules or merge? Is there a better name to put them both under?
  • We need to get a pull request flow across repos. I read etagify sources, great work!

This comment has been minimized.

Sign in to view

Copy link
@lloyd

lloyd Mar 27, 2012

Author Contributor

I agree, cachify and etagify are similar. I can imagine a nice package which bakes them together and is the one stop shop to make your app awesome w.r.t. client side caching.

I wonder if we let it simmer a bit and see how this stuff all works out, then start figgerin' out how to mix em up?

This comment has been minimized.

Sign in to view

Copy link
@ozten

ozten Mar 27, 2012

Contributor

Ya, that makes a lot of sense.
"express": "2.5.0",
"iconv": "1.1.3",
"jwcrypto": "0.1.1",
Expand Down

2 comments on commit edd34eb

@ozten
Copy link
Contributor

@ozten ozten commented on edd34eb Mar 26, 2012

Choose a reason for hiding this comment

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

I think this pull request needs to update the tests, also:

✗
/home/ozten/browserid/tests/cache-header-tests.js:78
        var etag = r.headers['etag'].replace(/"$/, "bogus\"");
                                     ^
TypeError: Cannot call method 'replace' of undefined
    at Object.<anonymous> (/home/ozten/browserid/tests/cache-header-tests.js:78:38)
    at run (/home/ozten/browserid/node_modules/vows/lib/vows/suite.js:128:31)
    at EventEmitter.<anonymous> (/home/ozten/browserid/node_modules/vows/lib/vows/suite.js:201:40)
    at EventEmitter.emit (events.js:88:20)
    at /home/ozten/browserid/node_modules/vows/lib/vows/context.js:38:39
    at /home/ozten/browserid/node_modules/vows/lib/vows/context.js:46:29
    at ClientRequest.<anonymous> (/home/ozten/browserid/tests/cache-header-tests.js:40:5)
    at ClientRequest.emit (events.js:67:17)
    at HTTPParser.onIncoming (http.js:1261:11)
    at HTTPParser.onHeadersComplete (http.js:102:31)
>>> Now Running: Front end unit tests under PhantomJS
✗ · · verifier (22561): error: uncaughtException pid=22561, uid=1000, gid=1000, cwd=/home/ozten/browserid, execPath=/usr/local/bin/node, version=v0.6.12, argv=[node, /home/ozten/browserid/bin/verifier], rss=29315072, heapTotal=27286784, heapUsed=14417048, loadavg=[0.38671875, 0.35107421875, 0.21923828125], uptime=336218.575315077, trace=[column=11, file=net.js, function=errnoException, line=646, method=null, native=false, column=26, file=net.js, function=Array.0, line=747, method=0, native=false, column=40, file=node.js, function=EventEmitter._tickCallback, line=192, method=_tickCallback, native=false], stack=[Error: listen EADDRINUSE,     at errnoException (net.js:646:11),     at Array.0 (net.js:747:26),     at EventEmitter._tickCallback (node.js:192:40)]


✗ Errored » Asynchronous Error
    in frontend-tests
    in undefined✗ Errored » 2 honored ∙ 1 broken ∙ 1 errored

@ozten
Copy link
Contributor

@ozten ozten commented on edd34eb Mar 26, 2012

Choose a reason for hiding this comment

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

Unable to manually test locally, due to cookies issue which also exists on the dev branch.

r- please fix unit tests.

Please sign in to comment.