Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add chunks to html scripts that are loaded on Build Time Render #186

Merged
merged 11 commits into from Aug 6, 2019

Conversation

@rorticus
Copy link
Contributor

@rorticus rorticus commented Jul 22, 2019

Type: feature

The following has been addressed in the PR:

Description:

During a BTR build, the page may loads an additional scripts like async bundles. These additional scripts are now captured and injected into the emitted html pages if you use the StateHistory history manager.

Resolves #179

@rorticus rorticus requested review from agubler and matt-gadd Jul 22, 2019
@@ -53,6 +54,8 @@ function genHash(content: string): string {
.substr(0, 20);
}

const ignoreAdditionalChunks = ['main.js', 'bootstrap.js', 'runtime.js'];
Copy link
Contributor Author

@rorticus rorticus Jul 22, 2019

Choose a reason for hiding this comment

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

runtime.js is here because the tests use it. It may be better to remove this and migrate the tests to use bootstrap.js instead.

@codecov
Copy link

@codecov codecov bot commented Jul 22, 2019

Codecov Report

Merging #186 into master will increase coverage by 0.01%.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   86.02%   86.04%   +0.01%     
==========================================
  Files          41       41              
  Lines        1861     1885      +24     
  Branches      501      504       +3     
==========================================
+ Hits         1601     1622      +21     
- Misses        260      263       +3
Impacted Files Coverage Δ
src/build-time-render/BuildTimeRender.ts 98.94% <89.28%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cbd602...de4ef45. Read the comment docs.

content = content.replace(/http:\/\/localhost:\d+\//g, '');
if (this._useHistory) {
styles = styles.replace(/url\("(?!(http(s)?|\/))(.*?)"/g, `url("${getPrefix(pathValue)}$3"`);
content = content.replace(/src="(?!(http(s)?|\/))(.*?)"/g, `src="${getPrefix(pathValue)}$3"`);
script = generateBasePath(pathValue);

const jsCoverage = await page.coverage.stopJSCoverage();

Choose a reason for hiding this comment

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

would it be possible to just query script tags for this rather than using puppeteers jscoverage? this will make it more portable for when we support jsdom again..

@rorticus
Copy link
Contributor Author

@rorticus rorticus commented Aug 2, 2019

Need to fix up the tests on this, but I've updated to query script tags on the page as well as preload css files!

@rorticus
Copy link
Contributor Author

@rorticus rorticus commented Aug 5, 2019

  • <script> tags don't use `async` anymore
  • CSS files are inserted now
  • Script / CSS files included in entries are excluded from additionalCss/Scripts

Network graph for dojo-create-app with a simple block,

image

const cssFile = this._manifest[entry.replace('.js', '.css')];
if (cssFile) {
html = html.replace(`<link href="${prefix}${cssFile}" rel="stylesheet">`, '');
css = `${css}<link rel="stylesheet" href="${prefix}${cssFile}" media="none" onload="if(media!='all')media='all'" />`;
css = `${css}<link rel="stylesheet" href="${prefix}${cssFile}" />`;
Copy link
Contributor Author

@rorticus rorticus Aug 5, 2019

Choose a reason for hiding this comment

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

Removed media="none" onload="if(media!='all')media='all'" to improve load performance.

@@ -124,9 +141,19 @@ export default class BuildTimeRender {
blockScripts.forEach((blockScript, i) => {
html = html.replace(
'</body>',
`<script type="text/javascript" src="${scriptPrefix}${blockScript}" async="true"></script></body>`
`<script type="text/javascript" src="${scriptPrefix}${blockScript}"></script></body>`
Copy link
Contributor Author

@rorticus rorticus Aug 5, 2019

Choose a reason for hiding this comment

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

Removed async="true" to increase the priority of script so the browser downloads it right away.

);
});
additionalScripts
.sort((script1, script2) => {
Copy link
Contributor Author

@rorticus rorticus Aug 5, 2019

Choose a reason for hiding this comment

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

This sort forces main. scripts to be loaded last. If main.12345.bundle.js is loaded and executed before the additional scripts, the additional scripts have a chance to be loaded by the main bundle anyways, thus negating all this preloading.

Choose a reason for hiding this comment

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

can we not make this an exact match? otherwise it would pick up say main.container.js?

);
});
additionalScripts
.sort((script1, script2) => {

Choose a reason for hiding this comment

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

can we not make this an exact match? otherwise it would pick up say main.container.js?

@rorticus
Copy link
Contributor Author

@rorticus rorticus commented Aug 5, 2019

@matt-gadd Done!

@rorticus
Copy link
Contributor Author

@rorticus rorticus commented Aug 5, 2019

Converted preloaded sources to use <link rel="preload". Here is a screenshot of an audit of the dojo site homepage,

image

@rorticus rorticus merged commit 4c64293 into dojo:master Aug 6, 2019
5 checks passed
@rorticus rorticus deleted the issue-179 branch Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants