Skip to content

Some small optimizations for stylesheet injection #328

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

Closed
wants to merge 1 commit into from
Closed

Some small optimizations for stylesheet injection #328

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: 2 additions & 4 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,6 @@ class MiniCssExtractPlugin {
'}',
'var linkTag = document.createElement("link");',
'linkTag.rel = "stylesheet";',
'linkTag.type = "text/css";',
Copy link
Member

Choose a reason for hiding this comment

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

Why we need remove this? Not sure it is right idea

Copy link
Member

Choose a reason for hiding this comment

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

Removing text/css good for HTML5, but some browsers (include mobile) still have bad supporting HTML5

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep compatibility as long as we can. Third-world countries and the corporations there can and usually are locked on older "legacy” devices or browsers

Copy link
Author

@developit developit Jan 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The default value of the type attribute has always been text/css. The only difference in HTML5 was making it pass validation when omitted. There is no functionality change with regard to this behaviour between the two specs.

You can test this yourself - load this webpage in IE6:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html lang="en">
  <head>
    <title>HTML 4 Strict</title>
    <link rel="stylesheet" href="style.css">
    <!-- make sure style.css exists and and is not empty -->
  </head>  
  <body>
    <script>
      alert(document.styleSheets.length);
    </script>
  </body>
</html>

alert 1 in IE6

(also worth noting: an argument for specifying type on <link> tags implies we should do the same for <script>, which is universally regarded as unnecessary)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done

'linkTag.onload = resolve;',
'linkTag.onerror = function(event) {',
Template.indent([
Expand All @@ -368,7 +367,7 @@ class MiniCssExtractPlugin {
'linkTag.href = fullhref;',
crossOriginLoading
? Template.asString([
`if (linkTag.href.indexOf(window.location.origin + '/') !== 0) {`,
`if (linkTag.href.indexOf(location.origin + '/') !== 0) {`,
Copy link
Member

Choose a reason for hiding this comment

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

Always need window, you can have scope problem in some rare cases

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, don't rely on global scope being there. explicit, no implicit is always my suggestion

Copy link
Author

Choose a reason for hiding this comment

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

What scope problem are you referring to?

var location = 1 or location=1 in global scope redirect the page, as does window.location=1 - it's exceptionally unlikely the value would be redefined. It could be shadowed, but that's equally true for any variable including window.

Copy link

Choose a reason for hiding this comment

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

Note that the code already uses document rather than window.document.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point... we should side with implicit or explicit conventions throughout

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve had scope issues before. Rarely but they do happen. Even if wasted bytes. This plugin gets like 5mil downloads a week. Eventually complaints would roll in. We are talking race conditions.

Stupid question, what if we declared window as a variable, then w.document or w.location? One way to save bytes maybe. Can’t speak if race conditions would come of this never tried

Copy link

Choose a reason for hiding this comment

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

We are talking race conditions.

I don't see a race condition here.

what if we declared window as a variable, then w.document or w.location? One way to save bytes maybe.

If gzip's involved, that uses more bytes than window.document.

Fwiw I think omitting window is fine.

Template.indent(
`linkTag.crossOrigin = ${JSON.stringify(
crossOriginLoading
Expand All @@ -377,8 +376,7 @@ class MiniCssExtractPlugin {
'}',
])
: '',
'var head = document.getElementsByTagName("head")[0];',
'head.appendChild(linkTag);',
'document.head.appendChild(linkTag);',
Copy link
Member

Choose a reason for hiding this comment

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

Hm IE9 doesn't support getElementsByTagName?

Copy link
Contributor

Choose a reason for hiding this comment

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

@evilebottnawi looks safe to me?

https://caniuse.com/#search=getElementsByTagName

Copy link
Member

Choose a reason for hiding this comment

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

@ScriptedAlchemy for me too, so i asked question why it was changed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah good point. Though both of them do work with old I.E.

However I’m unsure how widely supported ‘document.head’ is especially in older browsers

Copy link
Author

Choose a reason for hiding this comment

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

document.head is supported in IE9+.
https://developer.mozilla.org/en-US/docs/Web/API/Document/head#Browser_compatibility

Copy link
Author

Choose a reason for hiding this comment

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

Looks like Webpack core has also switched over to using document.head:
webpack/webpack#8467

]),
'}).then(function() {',
Template.indent(['installedCssChunks[chunkId] = 0;']),
Expand Down