fix: Promise pollyfil loaded before webpack bundle #127

rkostrzewski · 2017-06-20T23:31:15Z

Closes #101.

Tested in Chrome on desktop & mobile and IE 11. Chrome doesn't complain about document.write (ofc I've used typeof fetch === "function"). Rendering is not blocked in IE. 😄

lukeed · 2017-06-20T23:34:55Z

src/lib/webpack-config.js

 		minify: true,
 		stripPrefix: config.cwd,
 		staticFileGlobsIgnorePatterns: [
+			/polyfills\.js$/,


Did you ignore this because all serviceWorker-compatible browsers don't need a fetch or Promise polyfill?

(^ I'm not certain that that's 100% true)

From what I understand serviceWorker implementation requires fetch implementation which in turn requires Promise implementation. Based on FetchEvent. I could be wrong tough.

Ah, right. I believe that's the case too 👍 You upgraded my 90% certainty to 95% 😉

lukeed · 2017-06-20T23:38:04Z

src/resources/template.html

 		<%= htmlWebpackPlugin.options.ssr({
 			url: '/'
 		}) %>
+		<script>"undefined"==typeof fetch&&document.write('<script src="<%= htmlWebpackPlugin.files.publicPath + "polyfills.js" %>"><\/script>')</script>


How about (window.fetch===void 0) && ...?

Sounds great! I'll change it to window.fetch==void 0&&document.write(...) (5 less characters & html webpack plugin doesn't minify inline script 😢 )

Lol okay. I was just going for perf since !== void 0 is faster than typeof === 'undefined'

if we're changing, maybe just go with window.fetch || document.write(..).

developit

This is awesome - I was trying to do the exact same thing locally before I ended up going with the alias+ensure() setup but I couldn't get it working. It pains me ever so slightly to have a blocking document.write of a script in our HTML though - any thoughts on working around that?

developit · 2017-06-21T14:40:58Z

src/resources/template.html

 		<%= htmlWebpackPlugin.options.ssr({
 			url: '/'
 		}) %>
+		<script>"undefined"==typeof fetch&&document.write('<script src="<%= htmlWebpackPlugin.files.publicPath + "polyfills.js" %>"><\/script>')</script>


if we're changing, maybe just go with window.fetch || document.write(..).

developit · 2017-06-21T14:44:08Z

Randomly commented in my review - do you guys think we could do something like this to work around using a blocking document.write(<script>)?

<body>
  <script src="/bundle.js" defer></script>
  <script>window.fetch || document.body.appendChild(document.createElement('script')).src='/polyfills.js';</script>
</body>

ethanroday · 2017-06-21T15:38:50Z

@developit Why not split out Promise from the rest of the polyfills and then take the approach you mentioned?

<script>window.Promise || document.body.appendChild(....</script>

Then the rest of the polyfills could just be loaded as normal from webpack.

thangngoc89 · 2017-06-21T16:14:08Z

@ethanroday currently, preact-cli only includes Promise and fetch. And you need promise supports for fetch anyway. So no splitting needed

lukeed · 2017-06-21T18:48:31Z

Doesn't it have to be blocking in order to ensure it loads before bundle?

developit · 2017-06-22T01:58:18Z

@lukeed I wasn't sure if the async script (via createElement) would defer the defer script since those wait for document completion.

rkostrzewski · 2017-06-22T11:20:48Z

@developit so I've done my research.

document.body.appendChild(document.createElement('script')) doesn't block deferred scripts (kinda makes sense because defer only works for initial html, which document.write kinda mutates) so document.write has to stay. Good thing is document.write(<script>) does not block render (tested in Chrome & IE 11).

I've changed async to defer to preload bundle.js and added parsing blocking document.write. Tested in all IE >=9, latest Edge, Chrome, Firefox and android 4 browser. All is well 😄

lukeed · 2017-06-22T17:28:43Z

@rkostrzewski Makes sense, that's what I had "remembered" too.

However, I'm worried that changing from async to defer will hurt our load time. It shouldn't matter which attribute the main bundle is using since document.write is first & blocks, right?

rkostrzewski · 2017-06-22T18:09:37Z

@lukeed perf should be pretty much the same for async & defer when using only one script. Additionally, <script> tag doesn't block <script async> as they are executed as soon as they are downloaded and stack is empty. bundle.js is much smaller than polyfills.js for initial project which means that it will most likely be downloaded earlier and executed earlier <- Promise doesn't exist yet - require.ensure & import() fail.

tl;dr async is random 😛

lukeed · 2017-06-22T18:11:20Z

Haha okay 😆 Thanks!

rkostrzewski · 2017-06-22T18:28:53Z

src/resources/template.html

 		<% for (var chunk of webpack.chunks) { %>
+		 <% if (chunk.names.length === 1 && chunk.names[0] === 'polyfills') continue; %>
 			<% for (var file of chunk.files) { %>
 				<% if (htmlWebpackPlugin.options.preload && file.match(/\.(js|css)$/)) { %>


BTW htmlWebpackPlugin.options.preload comes from argv.preload which is a hidden switch that can't be used. Gonna check whole codebase for hidden switches sometime soon.

# Conflicts: # package.json

lukeed

Didn't check out locally but looks good to me!

rkostrzewski · 2017-06-25T07:08:58Z

@lukeed gonna fix regex coz I just noticed it is wrong ([bundle|polyfills] should have been (bundle|polyfills). Also package-lock.json should not been comitted I guess.

reznord · 2017-06-25T07:13:51Z

@rkostrzewski you are using npm5.x? I thought it wasn't that stable and it has many issues.

rkostrzewski · 2017-06-25T07:14:58Z

@reznord works good enough for me. Hadn't had any issues yet.

reznord · 2017-06-25T07:16:21Z

I was had issues with it when I was working with react-native, so thought of upgrading later.

reznord · 2017-06-25T07:16:39Z

any issues which I can take over?

rkostrzewski · 2017-06-25T10:43:44Z

@reznord if you want feel free take anything with help wanted label 😄

fix: Promise pollyfil loaded before webpack bundle

119eb45

rkostrzewski requested review from developit and lukeed June 20, 2017 23:31

lukeed reviewed Jun 20, 2017

View reviewed changes

developit reviewed Jun 21, 2017

View reviewed changes

fix: Promise pollyfil loaded before webpack bundle

b81227a

rkostrzewski commented Jun 22, 2017

View reviewed changes

Merge branch 'master' into fix/promise-polyfill

6395e3a

# Conflicts: # package.json

lukeed approved these changes Jun 25, 2017

View reviewed changes

rkostrzewski added this to the 1.2.0 milestone Jun 25, 2017

fix: Change lousy regex + remove package-lock.json

9532636

rkostrzewski merged commit 5a56336 into master Jun 25, 2017

rkostrzewski deleted the fix/promise-polyfill branch June 25, 2017 07:34

developit mentioned this pull request Jun 28, 2017

Promise polyfill is not applied #101

Closed

yaodingyd mentioned this pull request Dec 7, 2018

remove script-ext-html-webpack-plugin #678

Closed

yaodingyd mentioned this pull request Dec 9, 2018

remove script-ext-html-webpack-plugin and add script loading option #681

Closed

Uh oh!

fix: Promise pollyfil loaded before webpack bundle #127

fix: Promise pollyfil loaded before webpack bundle #127

Uh oh!

Conversation

rkostrzewski commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

developit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

developit commented Jun 21, 2017

Uh oh!

ethanroday commented Jun 21, 2017

Uh oh!

thangngoc89 commented Jun 21, 2017

Uh oh!

lukeed commented Jun 21, 2017

Uh oh!

developit commented Jun 22, 2017

Uh oh!

rkostrzewski commented Jun 22, 2017

Uh oh!

lukeed commented Jun 22, 2017

Uh oh!

rkostrzewski commented Jun 22, 2017

Uh oh!

lukeed commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeed left a comment

Choose a reason for hiding this comment

Uh oh!

rkostrzewski commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reznord commented Jun 25, 2017

Uh oh!

rkostrzewski commented Jun 25, 2017

Uh oh!

reznord commented Jun 25, 2017

Uh oh!

reznord commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkostrzewski commented Jun 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rkostrzewski commented Jun 25, 2017 •

edited

Loading

reznord commented Jun 25, 2017 •

edited

Loading