Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork388
fix(index): dynamic css loading support for older browsers#134
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
jsf-clabot commentedMay 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
@gaterking can you provide minimum reproducible test repo and browser version to ensure it is does not works for all, thanks! |
@evilebottnawihttp://pie.gd/test/script-link-events/ |
@gaterking also please accept CLA |
@gaterking can you faced this problem ? |
src/index.js Outdated
| @@ -338,8 +338,10 @@ class MiniCssExtractPlugin { | |||
| ]), | |||
| '}', | |||
| 'var linkTag = document.createElement("link");', | |||
| 'var isOldWebKit = Number(navigator.userAgent.replace(/.*AppleWebKit\\/(\\d+)..*/, "$1")) < 536;', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we can find better solution for check this, can you investigate how this do examplejquery or other frontend libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At present, old browser is less. I only use regex to match AppleWebKit 536, bescause I think it will cover most of old browser. There is another solution to check version:
https://github.com/filamentgroup/loadCSS/blob/master/src/onloadCSS.js
As bellow is another solution to check if css is loaded:
http://www.backalleycoder.com/2011/03/20/link-tag-css-stylesheet-load-event/
https://www.zachleat.com/web/load-css-dynamically/
Another question: Why we need to wait resolve for css chunks is loaded? CSS chunkses are appended in sequence, they can't impact each other. The waiting will increase js render time inversely. There is so less case we need to caculate element style when the the self css is loaded but next css is not loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gaterkinghttp://www.backalleycoder.com/2011/03/20/link-tag-css-stylesheet-load-event/ looks universal, but create new dom node looks overload.
Give me time on thinking about best solution, ping be tomorrow and we find better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://github.com/guybedford/require-css/blob/master/css.js
A CSS Loader plugin for RequireJS which can be refered to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gaterking looks solid, maybe we can port solution here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it's solid. We can remove some inspections such as ie or low version firefox. Webpack 4 don't support ie6 to 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gaterking we can avoid part of code for oldie
@evilebottnawi Yes, I faced it in my vue project. Android 4.3 browser will fail, bescause some lazy load modules with css chunk not resolve. |
@gaterking looks good solution, but i think we should search better solution for detecting this |
This also happened to be a blocker for a smartphone-specific project at work. I'll link this to the person involved so he can check if this works 🤔 |
Note that it's important to know when a dynamically inserted style is loaded. Not knowing this leads to FOUC, which can be disastrous if you're doing things like |
index.js |
what the next step for this PR? |
| @@ -349,7 +362,25 @@ class MiniCssExtractPlugin { | |||
| 'reject(err);', | |||
| ]), | |||
| '};', | |||
| 'linkTag.href = fullhref;', | |||
| '} else {', | |||
| 'var errorTimeout = 60000;', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about is loading more than60000 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's a timeout threshold for css loading. To make sure chunk is resolved, I think we need a stop time to clear setInterval when the css loaded fail.
hxlniada commentedAug 23, 2018
may be this is a better solution:https://stackoverflow.com/a/5371426 |
/cc@gaterking |
I think this PR is not necessary for most developer, maybe we can close it. If somebody need to support old browser, I suggest to clone own repo. |
lili21 commentedOct 16, 2018
@evilebottnawi what do you think? |
lili21 commentedOct 16, 2018
@hxlniada It's a neat solution, just need a little change. ...varimg=document.createElement('img');img.onerror=function(e){varrequest=event&&event.target&&event.target.src||fullhref;vari=document.styleSheets.length;while(i--){if(document.styleSheets[i].href===request){returnresolve();}}varerr=newError('Loading CSS chunk '+chunkId+' failed.\n('+request+')');err.request=request;reject(err);};img.src=fullhref;... |
gaterking commentedNov 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
#299 similar PR |
| 'linkTag.href = fullhref;', | ||
| "// old webkit's would claim to have onload, but didn't really support it", | ||
| '// https://github.com/kriszyp/xstyle/blob/master/core/load-css.js', | ||
| 'var webkitVersion = navigator.userAgent.match(/AppleWebKit\\/(\\d+\\.?\\d*)/);', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This extra check affects only 0.02% of the internet. I don't think it's worth including this workaround of everybody.
| '// most browsers support this onload function now', | ||
| 'linkTag.onload = function(){', | ||
| '// cleanup', | ||
| 'linkTag.onload = null;', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
indent
| 'var startTime = Date.now();', | ||
| 'var interval = setInterval(function(){', | ||
| 'if(linkTag.style){', | ||
| 'clearInterval(interval);', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
indent
| 'clearInterval(interval);', | ||
| 'resolve();', | ||
| '}', | ||
| 'if(!linkTag.style && (Date.now() - startTime) > errorTimeout){', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The support for onload and onerror differ. Make sure to handle them separately. Use a setTimeout for error handling and don't merge it into the setInterval for onload.
| 'var errorTimeout = 60000;', | ||
| 'var startTime = Date.now();', | ||
| 'var interval = setInterval(function(){', | ||
| 'if(linkTag.style){', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of polling this seem to be a better solution for this problem:https://github.com/webpack-contrib/mini-css-extract-plugin/pull/299/files
codecovbot commentedMay 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #134 +/- ##====================================== Coverage 0.00% 0.00% ====================================== Files 3 3 Lines 241 241 Branches 49 49 ====================================== Misses 198 198 Partials 43 43
Continue to review full report at Codecov.
|
This PR contains a:
Motivation / Use-Case
Breaking Changes
Additional Info
In old browser, such as android 4.3, link tag not support 4.3. Although these devices is few, the onload callback resolve function will not be called. This PR ignore onload for these devices and resolve immediately.