Skip to content

Use nomodule to conditionally load fetch+promise #833

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.

Sign up for GitHub

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

Merged
prateekbh merged 1 commit into from
Jul 10, 2019
Merged

Use nomodule to conditionally load fetch+promise #833

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions packages/cli/lib/resources/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,17 @@
<%= htmlWebpackPlugin.options.ssr() %>
<% if (webpack.assets.filter(entry => entry.name.match(/bundle(\.\w{5})?.esm.js$/)).length > 0) { %>
<% /* Fix for safari < 11 nomodule bug. TODO: Do the following only for safari. */ %>
<script>!function(){var e=document,t=e.createElement("script");if(!("noModule"in t)&&"onbeforeload"in t){var n=!1;e.addEventListener("beforeload",function(e){if(e.target===t)n=!0;else if(!e.target.hasAttribute("nomodule")||!n)return;e.preventDefault()},!0),t.type="module",t.src=".",e.head.appendChild(t),t.remove()}}();</script>
<script nomodule>!function(){var e=document,t=e.createElement("script");if(!("noModule"in t)&&"onbeforeload"in t){var n=!1;e.addEventListener("beforeload",function(e){if(e.target===t)n=!0;else if(!e.target.hasAttribute("nomodule")||!n)return;e.preventDefault()},!0),t.type="module",t.src=".",e.head.appendChild(t),t.remove()}}();</script>
<script crossorigin="anonymous" src="<%= htmlWebpackPlugin.files.publicPath %><%= webpack.assets.filter(entry => entry.name.match(/bundle(\.\w{5})?.esm.js$/))[0].name %>" type="module"></script>
<%
/*Fetch and Promise polyfills are not needed for browsers that support type=module
Please re-evaluate below line if adding more polyfills.*/
%>
<script nomodule>window.fetch||document.write('<script src="<%= htmlWebpackPlugin.files.chunks["polyfills"].entry %>"><\/script>')</script>
<script nomodule src="<%= htmlWebpackPlugin.files.chunks["polyfills"].entry %>"></script>
<script nomodule defer src="<%= htmlWebpackPlugin.files.chunks['bundle'].entry %>"></script>
<% } else { %>
<script <%= htmlWebpackPlugin.options.scriptLoading %> src="<%= htmlWebpackPlugin.files.chunks['bundle'].entry %>"></script>
<script>window.fetch||document.write('<script src="<%= htmlWebpackPlugin.files.chunks["polyfills"].entry %>"><\/script>')</script>
<script nomodule src="<%= htmlWebpackPlugin.files.chunks["polyfills"].entry %>"></script>
Copy link
Member

@prateekbh prateekbh Jul 10, 2019

Choose a reason for hiding this comment

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

Why does this need nomodule?
This section only executes if we --no-esm is used. So i guess its better with window.fetch||document.write?

Copy link
Member

@ForsakenHarmony ForsakenHarmony Jul 10, 2019

Choose a reason for hiding this comment

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

it still works though and no javascript has to be executed for it?

Copy link
Member

@prateekbh prateekbh Jul 10, 2019

Choose a reason for hiding this comment

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

yep but if someone is explicitly telling you, they don't want --esm than this just worsens the performance for them. WDYT?

Copy link
Member

@ForsakenHarmony ForsakenHarmony Jul 10, 2019

Choose a reason for hiding this comment

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

it worsens performance?

Copy link
Member

@prateekbh prateekbh Jul 10, 2019

Choose a reason for hiding this comment

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

Like the nomodule compatible browsers might not need the polyfills but this will still give it to them.
Or am i missing something?

Copy link
Member

@ForsakenHarmony ForsakenHarmony Jul 10, 2019

Choose a reason for hiding this comment

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

This switches from using document.write() for polyfill injection to using <script nomodule>. It's not a perfect match, so it'll be loading fetch+Promise in Safari 10 where they're natively supported, but the performance benefits in other browsers seem worthwhile.

Copy link
Member

@prateekbh prateekbh Jul 10, 2019

Choose a reason for hiding this comment

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

/me dumb.
/me hides in a corner

Copy link
Member

@prateekbh prateekbh Jul 10, 2019

Choose a reason for hiding this comment

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

sorry didn't read it through my bad totally

<% } %>
</body>
</html>