Feature/do 1484 enable cache for redirects #1069
Conversation
| server.use(s3Cache); | ||
|
|
||
| if (process.env.ENABLE_REDIRECT_CACHE.toLowerCase() === 'true'){ | ||
| server.use(prerender.removeScriptTags()); |
There was a problem hiding this comment.
This is used in both conditions so it can be pulled out
| server.use({ | ||
| requestReceived: function(req, res, next) { | ||
| if(req.method !== 'GET' && req.method !== 'HEAD') { | ||
| console.log("skipping requestReceived from S3 Cache... ") |
There was a problem hiding this comment.
Minor point but I would remove this console log. It could be useful during local dev but in a prod system it wouldn't be very helpful (e.g. no logging of which request is being skipped).
| }, function (err, result) { | ||
|
|
||
| if (!err && result) { | ||
| console.log(result.Metadata); |
There was a problem hiding this comment.
This console log is also questionable in terms of debugging usefulness, I'd probably remove it too
| headerMatch = /<meta[^<>]*(?:name=['"]prerender-header['"][^<>]*content=['"]([^'"]*?): ?([^'"]*?)['"]|content=['"]([^'"]*?): ?([^'"]*?)['"][^<>]*name=['"]prerender-header['"])[^<>]*>/gi, | ||
| head = req.prerender.content.toString().split('</head>', 1).pop(), | ||
| // statusCode = 200, | ||
| match; |
There was a problem hiding this comment.
Just for clarity I'd put a let on each line rather than using the comma syntax. I can be a bit hard to read especially with the long regexes 😅
There was a problem hiding this comment.
Also if a variable isn't going to change you should use the const keyword rather than let
| console.log("metaTagStatusCode: " + metaTagStatusCode); | ||
| } | ||
|
|
||
| while (match = headerMatch.exec(head)) { |
There was a problem hiding this comment.
Why is this in a while loop while the one above is an if statement? 🤔
|
|
||
| // Skip caching for the http response codes not in the list, such as 404 | ||
| if ( ! statusCodesToCache.includes(metaTagStatusCode.toString()) ) { | ||
| console.log("metaTagStatusCode " + metaTagStatusCode + " is not in the cachable code list. Returning without caching."); |
There was a problem hiding this comment.
Probably not a useful log either. Also fun JS syntax fact, it's generally preferred to use string literals rather than +: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
This would become:
console.log(`metaTagStatusCode ${metaTagStatusCode} is not in the cachable code list. Returning without caching.`);
|
|
||
| // Override req.prerender.statusCode with the StatusCode returned via the meta tag. | ||
| // If metaTagStatusCode is not in the statusCodesToCache array or req.prerender.statusCode is not 200, then this line wouldn't be reached. Therefore no if condition for this overriding is needed. | ||
| req.prerender.statusCode = metaTagStatusCode; |
There was a problem hiding this comment.
What's the purpose of the metaTagStatusCode variable, why not just assign to req.prerender.statusCode directly?
| // Override req.prerender.statusCode with the StatusCode returned via the meta tag. | ||
| // If metaTagStatusCode is not in the statusCodesToCache array or req.prerender.statusCode is not 200, then this line wouldn't be reached. Therefore no if condition for this overriding is needed. | ||
| req.prerender.statusCode = metaTagStatusCode; | ||
| console.log("Caching the object with statusCode " + req.prerender.statusCode); |
There was a problem hiding this comment.
Might be worth stating which object is being cached (url key maybe?)
| httpreturncode: req.prerender.statusCode.toString() | ||
| } | ||
|
|
||
| if (location) { |
There was a problem hiding this comment.
I feel like this could be done earlier on, it feels a bit out of place referencing the location variable here. I reckon you could change this so location is scoped inside an existing if statement above rather than being scoped to the function
…after httpHeader is run
| // s3cache plugin - https://github.com/prerender/prerender-aws-s3-cache/blob/98707fa0f787de83aa41583682cd2c2d330a9cca/index.js | ||
| requestReceived: function(req, res, next) { | ||
| if(req.method !== 'GET' && req.method !== 'HEAD') { | ||
| \ return next(); |
There was a problem hiding this comment.
I don't think this slash should be here 🤔
Description of the proposed changes
Screenshots (if applicable)
Other solutions considered (if any)
Notes to PR author
Notes to reviewers
Incorporated the below existing plug-ins
🛈 When you've finished leaving feedback, please add a final comment to the PR tagging the author, letting them know that you have finished leaving feedback