fix issue with preloading wrong assets on pages #141
Conversation
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
========================================
Coverage ? 92.3%
========================================
Files ? 13
Lines ? 507
Branches ? 89
========================================
Hits ? 468
Misses ? 39
Partials ? 0
Continue to review full report at Codecov.
|
| return assetsToPreload; | ||
| } | ||
| if (links.stylesheet && links.stylesheet.url) { | ||
| assetsToPreload += `<${links.stylesheet.url}>; rel="preload"; as="style"; nopush,`; |
There was a problem hiding this comment.
I think we might end up with a dangling comma here.
There was a problem hiding this comment.
What do you mean? We need the comma between two preloaded assets.
There was a problem hiding this comment.
Probably a trailing comma when the primary fragment doesn't have a JS?
There was a problem hiding this comment.
Yes and browsers does ignore it.. Don't think we need to do anything here. Thoughts?
lib/request-handler.js
Outdated
| @@ -118,7 +109,10 @@ module.exports = function processRequest (options, request, response) { | |||
| } | |||
| this.emit('response', request, statusCode, responseHeaders); | |||
| // Make resources early discoverable while processing HTML | |||
| assetsToPreload && response.setHeader('Link', assetsToPreload); | |||
| let preloadAssets = getAssetsToPreload(headers, request); | |||
There was a problem hiding this comment.
Oops, thought I had a config for eslint to capture this case.
This is cleaner approach and fixes the issue with preloading wrong assets for previously rendered page. This scenario is hard to test because of the way events work and you would need to loadtest the system to really create this scenario.