Some small optimizations for stylesheet injection #328

developit · 2018-12-14T15:12:09Z

This PR contains a:

Motivation / Use-Case

There are a few cases where the code injected into a bundle could be a bit smaller. I've addressed two really simple ones here - removing the type property from <link rel=stylesheet>, and using document.head (IE9+) instead of getElementsByTagName('head')[0].

Breaking Changes

This requires IE9+. Not sure if there's a browser support target for mini-css-extract-plugin that makes this prohibitive.

alexander-akait · 2018-12-14T15:18:51Z

src/index.js

@@ -353,7 +353,6 @@ class MiniCssExtractPlugin {
                  '}',
                  'var linkTag = document.createElement("link");',
                  'linkTag.rel = "stylesheet";',
-                  'linkTag.type = "text/css";',


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

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

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

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">  </head> <body> <script> alert(document.styleSheets.length); </script> </body> </html>

(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)

Nicely done

alexander-akait · 2018-12-14T15:19:16Z

src/index.js

@@ -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) {`,


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

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

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.

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

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

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

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.

alexander-akait · 2018-12-14T15:20:57Z

src/index.js

@@ -377,8 +376,7 @@ class MiniCssExtractPlugin {
                        '}',
                      ])
                    : '',
-                  'var head = document.getElementsByTagName("head")[0];',
-                  'head.appendChild(linkTag);',
+                  'document.head.appendChild(linkTag);',


Hm IE9 doesn't support getElementsByTagName?

@evilebottnawi looks safe to me?

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

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

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

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

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

alexander-akait · 2018-12-24T09:51:23Z

/cc @developit friendly ping

developit · 2019-01-08T20:14:12Z

Just to clarify my intention with this PR:

The code generated here is injected into a large percentage of websites. Each byte saved represents a nontrivial reduction in bundle size across the whole web.

alexander-akait

Good for me, but it is breaking change, some people can use plugin for IE8 and after merge we break their app, we can separate PR on two parts

Jessidhia · 2019-01-10T10:20:02Z

Do we really need to be this concerned about older browser support if #299 is not included?

Any browser that this PR would break is already broken by not having #299.

(EDIT: nevermind that as IE does support it; but webpack 2+ cannot bundle ES modules on IE<9 anyway because IE<9 don't have a working Object.defineProperty)

alexander-akait · 2019-01-10T10:30:54Z

@Kovensky hm, I forgot that we need to use webpack@4 for this plugin 😄 So webpack@4 doesn't support IE8, right?

Jessidhia · 2019-01-10T10:46:19Z

It depends on whether it's still possible to make a bundle that does not invoke Object.defineProperty (or __webpack_require__.d / __webpack_require__.n in this case; __webpack_require_.r can be faked in IE by es5-sham but not __webpack_require__.d or __webpack_require__.n)

alexander-akait · 2019-01-10T10:51:53Z

@Kovensky thanks for information, what do you think should we merge this as patch or major release?

Jessidhia · 2019-01-10T11:00:32Z

It looks like it is still possible if your code and entire dependency graph consists exclusively of CommonJS or AMD modules... I find that extremely unlikely.

Personally I'm OK with this break as it's overwhelmingly unlikely that it will break bundles that weren't already broken, I'm not sure if this might still affect people. It's not impossible that people might be intentionally using babel-loader + @babel/plugin-transform-modules-commonjs in loose: true mode to stop webpack from being able to see ES modules.

ScriptedAlchemy · 2019-01-12T21:55:06Z

What about a feature release? Unless we are really making a heavy impact change? Are we losing compatibility with current PRd changes?

Don’t mind major version bump, just throwing the question out there

ScriptedAlchemy · 2019-01-13T21:00:41Z

Yeah that’s fine then. Lets get this in as a major

This adds an new `insertInto` option that is optional and backards-compatible. Specifying a CSS selector allows the user to specify a DOM node into which the async-loaded <link> tags should be inserted. Closes webpack-contrib#370, related to webpack-contrib#328,webpack-contrib#331

codecov · 2020-06-01T16:43:16Z

Codecov Report

Merging #328 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #328   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           3       3           
  Lines         284     284           
  Branches       59      59           
======================================
  Misses        234     234           
  Partials       50      50

Impacted Files	Coverage Δ
src/index.js	`0.00% <ø> (ø)`

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 7d1e0ca...ae6f0f1. Read the comment docs.

sokra · 2020-06-01T17:21:25Z

People are using webpack 4 for ie8. That's a supported use case. ESM is not supported, by there is a ton of commonjs or amd code that can be used.

Some small optimizations for stylesheet injection

ae6f0f1

alexander-akait requested changes Dec 14, 2018

View reviewed changes

ooflorent approved these changes Jan 9, 2019

View reviewed changes

alexander-akait requested changes Jan 10, 2019

View reviewed changes

ScriptedAlchemy approved these changes Jan 13, 2019

View reviewed changes

brophdawg11 mentioned this pull request Apr 1, 2019

feat(ordering): add support for insertInto option #371

Closed

6 tasks

cap-Bernardito mentioned this pull request Oct 21, 2020

feat: linkType option #634

Merged

6 tasks

alexander-akait closed this in #634 Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some small optimizations for stylesheet injection #328

Some small optimizations for stylesheet injection #328

developit commented Dec 14, 2018

alexander-akait Dec 14, 2018

alexander-akait Dec 14, 2018

ScriptedAlchemy Dec 23, 2018

developit Jan 8, 2019 •

edited

Loading

ScriptedAlchemy Jan 9, 2019

alexander-akait Dec 14, 2018

ScriptedAlchemy Dec 23, 2018

developit Jan 8, 2019

jakearchibald Jan 9, 2019

ScriptedAlchemy Jan 9, 2019

ScriptedAlchemy Jan 9, 2019

jakearchibald Jan 9, 2019

alexander-akait Dec 14, 2018

ScriptedAlchemy Dec 24, 2018

alexander-akait Dec 25, 2018

ScriptedAlchemy Dec 29, 2018

developit Jan 8, 2019

developit Jan 9, 2019

alexander-akait commented Dec 24, 2018

developit commented Jan 8, 2019

alexander-akait left a comment

Jessidhia commented Jan 10, 2019 •

edited

Loading

alexander-akait commented Jan 10, 2019

Jessidhia commented Jan 10, 2019 •

edited

Loading

alexander-akait commented Jan 10, 2019

Jessidhia commented Jan 10, 2019 •

edited

Loading

ScriptedAlchemy commented Jan 12, 2019

ScriptedAlchemy commented Jan 13, 2019

codecov bot commented Jun 1, 2020 •

edited

Loading

sokra commented Jun 1, 2020

Some small optimizations for stylesheet injection #328

Some small optimizations for stylesheet injection #328

Conversation

developit commented Dec 14, 2018

Motivation / Use-Case

Breaking Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

developit Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Dec 24, 2018

developit commented Jan 8, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

Jessidhia commented Jan 10, 2019 • edited Loading

alexander-akait commented Jan 10, 2019

Jessidhia commented Jan 10, 2019 • edited Loading

alexander-akait commented Jan 10, 2019

Jessidhia commented Jan 10, 2019 • edited Loading

ScriptedAlchemy commented Jan 12, 2019

ScriptedAlchemy commented Jan 13, 2019

codecov bot commented Jun 1, 2020 • edited Loading

Codecov Report

sokra commented Jun 1, 2020

developit Jan 8, 2019 •

edited

Loading

Jessidhia commented Jan 10, 2019 •

edited

Loading

Jessidhia commented Jan 10, 2019 •

edited

Loading

Jessidhia commented Jan 10, 2019 •

edited

Loading

codecov bot commented Jun 1, 2020 •

edited

Loading