Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

fix issue with preloading wrong assets on pages #141

Merged
merged 2 commits into from
Apr 12, 2017
Merged

fix issue with preloading wrong assets on pages #141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69a6501
fix issue with preloading wrong assets on pages
vigneshshanmugam Apr 11, 2017
142c39f
change to const
vigneshshanmugam Apr 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 22 additions & 28 deletions lib/request-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,24 @@ const getCrossOriginHeader = (fragmentUrl, host) => {
return '';
};

// Early preloading of primary fragments assets to improve Performance
const getAssetsToPreload = ({ link }, { headers = {} }) => {
let assetsToPreload = '';
const links = parseLinkHeader(link);
// Handle Server rendered fragments without depending on assets
if (links == null) {
return assetsToPreload;
}
if (links.stylesheet && links.stylesheet.url) {
assetsToPreload += `<${links.stylesheet.url}>; rel="preload"; as="style"; nopush,`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might end up with a dangling comma here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? We need the comma between two preloaded assets.

Copy link

Choose a reason for hiding this comment

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

Probably a trailing comma when the primary fragment doesn't have a JS?

Copy link

Choose a reason for hiding this comment

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

But it's the existing behaviour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and browsers does ignore it.. Don't think we need to do anything here. Thoughts?

}
if (links['fragment-script'] && links['fragment-script'].url) {
const crossOrigin = getCrossOriginHeader(links['fragment-script'].url, headers.host);
assetsToPreload += `<${links['fragment-script'].url}>; rel="preload"; as="script"; nopush; ${crossOrigin}`;
}
return assetsToPreload;
};

/**
* Process the HTTP Request to the Tailor Middleware
*
Expand Down Expand Up @@ -42,33 +60,6 @@ module.exports = function processRequest (options, request, response) {

let shouldWriteHead = true;
let index = 0;
let assetsToPreload = '';

// Early preloading in browsers to improve the primary fragments
const getAssetsToPreload = (fragmentReq, fragment, statusCode, headers) => {
// Do not preload assets for non primary fragments
if (!fragment.primary) {
return;
}
const links = parseLinkHeader(headers.link);
// Remove the event listener once we are done capturing the necessary assets
// to make sure we are not exceeding EventListener's limit (10 default)
this.removeListener('fragment:response', getAssetsToPreload);

// Handle Server rendered fragments without depending on assets
if (links == null) {
return;
}
if (links.stylesheet && links.stylesheet.url) {
assetsToPreload += `<${links.stylesheet.url}>; rel="preload"; as="style"; nopush,`;
}
if (links['fragment-script'] && links['fragment-script'].url) {
const crossOrigin = getCrossOriginHeader(links['fragment-script'].url, request.headers.host);
assetsToPreload += `<${links['fragment-script'].url}>; rel="preload"; as="script"; nopush; ${crossOrigin},`;
}
};

this.on('fragment:response', getAssetsToPreload);

contextPromise.then((context) => {

Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

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

This can be const.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, thought I had a config for eslint to capture this case.

if (preloadAssets !== '') {
response.setHeader('Link', preloadAssets);
}
response.writeHead(statusCode, responseHeaders);
resultStream
.pipe(contentLengthStream)
Expand Down
4 changes: 2 additions & 2 deletions tests/tailor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ describe('Tailor', () => {
);

getResponse('http://localhost:8080/test').then((response) => {
assert.equal(response.headers.link, '<http://primary>; rel="preload"; as="style"; nopush,<http://primary>; rel="preload"; as="script"; nopush; crossorigin,');
assert.equal(response.headers.link, '<http://primary>; rel="preload"; as="style"; nopush,<http://primary>; rel="preload"; as="script"; nopush; crossorigin');
}).then(done, done);
});

Expand All @@ -783,7 +783,7 @@ describe('Tailor', () => {
mockTemplate.returns('<fragment primary src="http://fragment/"></fragment>');

getResponse('http://localhost:8080/test').then((response) => {
assert.equal(response.headers.link, '<http://localhost:8080>; rel="preload"; as="style"; nopush,<http://localhost:8080>; rel="preload"; as="script"; nopush; ,');
assert.equal(response.headers.link, '<http://localhost:8080>; rel="preload"; as="style"; nopush,<http://localhost:8080>; rel="preload"; as="script"; nopush; ');
}).then(done, done);
});

Expand Down