This repository has been archived by the owner. It is now read-only.

Fix to handle weird scenarios where the request urls to assets contain query string #36

Merged
merged 3 commits into from Sep 24, 2013

Conversation

Projects
None yet
3 participants
Contributor

infindex commented Jul 31, 2013

No description provided.

Member

jrgm commented Aug 1, 2013

So, for one thing, the tests no longer pass. Also a description of the problem you are trying to solve would help.

lib/connect-cachify.js
@@ -9,7 +9,8 @@ var crypto = require('crypto'),
format = require('./compat').format,
fs = require('fs'),
path = require('path'),
- und = require('underscore');
+ und = require('underscore'),
+ URL = require('url');
@ozten

ozten Sep 18, 2013

Member

variable holding the module should be lowercase.

lib/connect-cachify.js
@@ -114,7 +117,7 @@ exports.setup = function (assets, options) {
if (exists === true && hash === actualHash) {
resp.setHeader('Cache-Control', 'public, max-age=31536000');
- req.url = url;
+ req.url = url + '?' + req_url.query; // append back the query string
@ozten

ozten Sep 18, 2013

Member

Awesome! Can you add a test that demonstrates the problem that query string parameters introduce?

infindex added some commits Sep 20, 2013

- Fixed test failures
- Handle query parameter in development mode
- Adding tests to demonstrates the problem when query string parameters
  Are introduced
Contributor

infindex commented Sep 20, 2013

@jrgm - fixed the test failures...
@ozten - additional test cases added to demonstrate the query string problem. also fixed the variable name as suggested.

Also, added query string parameter handling in development mode and urls with Hash ids.

Thanks!
MG

+ root: '/tmp'
+ });
+ var link = cachify.cachify_js("/other", {hash: 'd41d8cd98f'});
+ test.equal(link, '<script src="/cachify/d41d8cd98f/other"></script>');
@ozten

ozten Sep 24, 2013

Member

Awesome, thanks for adding all these tests.

ozten added a commit that referenced this pull request Sep 24, 2013

Merge pull request #36 from infindex/master
Fix to handle weird scenarios where the request urls to assets contain query string

@ozten ozten merged commit 7cf3731 into mozilla:master Sep 24, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.