feat: support passing a `{Function}` (`options.insertInto`) #279

savelichalex · 2017-12-04T13:35:06Z

What kind of change does this PR introduce?
This is new feature, to use function in insertInto like options, that add more flexibility to loader. I.e. my case - I want to insert css into shadow root, but this is impossible right now (because ::shadow is deprecated)

Did you add tests for your changes?
Yes

If relevant, did you update the README?
Not sure about naming, need review first

Summary

Does this PR introduce a breaking change?
Pass all existed test cases

Other information

jsf-clabot · 2017-12-04T13:35:08Z

All committers have signed the CLA.

savelichalex · 2017-12-04T13:39:40Z

test/basicTest.js

+    requiredJS = [
+      "var el = document.createElement('div');",
+      "el.id = \"test-shadow\";",
+      // "var shadow = el.attachShadow({ mode: 'open' })", // sadly shadow dom not working in jsdom


I really want to test with shadow root at first, but jsdom not working with shadow dom. Maybe some day we can use smth like headless-chrome for testing

michael-ciniawsky

Thx so far, +1 for the feature, but the implementation needs rework and triage

michael-ciniawsky · 2017-12-04T17:03:25Z

lib/addStyles.js

@@ -23,7 +23,11 @@ var isOldIE = memoize(function () {
 	return window && document && document.all && !window.atob;
 });

-var getElement = (function (fn) {
+var getTargetFunction = function (target) {


getTargetFunction => getTarget

michael-ciniawsky · 2017-12-04T17:03:48Z

lib/addStyles.js

+  return document.querySelector(target);
+};
+
+var getElementCreator = function (fn) {


getElementCreator => getElement

michael-ciniawsky · 2017-12-04T17:04:13Z

lib/addStyles.js


+var getElement;


getElement => element

Not sure about this. Because in this place we create function nor element and then use it right there https://github.com/webpack-contrib/style-loader/blob/master/lib/addStyles.js#L154

michael-ciniawsky · 2017-12-04T17:05:05Z

test/basicTest.js

+    requiredJS = [
+      "var el = document.createElement('div');",
+      "el.id = \"test-shadow\";",
+      // "var shadow = el.attachShadow({ mode: 'open' })", // sadly shadow dom not working in jsdom


michael-ciniawsky · 2017-12-04T17:12:17Z

index.js

-		"var options = " + JSON.stringify(options),
-		"options.transform = transform",
+                "var insertIntoCallable;",
+                options.insertIntoCallable ? "insertIntoCallable = require(" + loaderUtils.stringifyRequest(this, "!" + path.resolve(options.insertIntoCallable)) + ")" : "",


This currently introduces a new options to the loader (options.insertIntoCallable) ? I would be much in favor to resolve this via type checking of options.insertInto e.g typeof options.InsertInto === 'function' instead of adding yet another option to the loader. Passing a {Function} is a good idea I planned to reduce the options of style-loader whenever possible in a similar way for the next major release since most of current options are kind of style-loader specific 'weirdness' due to technical debt and legacy support in the code.

Yeah, first time I'm trying to use existing options but loader have validation by schema and this is validation check for string. At second there is should be a path to file. As we can't use function directly we can't check it with typeof operator and have 100% guarantee that given string is path to file

Another idea: what if use .toString() for function and then eval (or inline) it. Not sure how it is safe

Yeah, first time I'm trying to use existing options but loader have validation by schema and this is validation check for string.

This can be fixed as the options validation is able to handle that via oneOf && typeof: 'function' now (the dependency in style-loader likely needs an update). Simply remove the the check for insertInto while developing for now and if needed I will help/take over at this point then

As we can't use function directly we can't check it with typeof operator and have 100% guarantee that given string is path to file

webpack.config.js

{ insertInto: require('path/to/custom/targetFn') // {Object} insertInto () { // {Function} return document.querySelector('#foo') } }

index.js L16+

// handler (within the loader phase) const insertInto = typeof options.insertInto === 'function' ? ... ... // pass the result to code block

Another idea: what if use .toString() for function and then eval (or inline) it. Not sure how it is safe

You can certainly try go in that direction since webpack will parse this code again so a function declaration as {String} should work (will be/should be 'evaluated' or parsed as {Function})

This can be fixed as the options validation is able to handle that via oneOf && typeof: 'function' now

Looks like oneOf in ajv not help, need to define new keyword

michael-ciniawsky · 2017-12-04T17:16:31Z

lib/addStyles.js

+        //}
+        // path/to/insertInto.js
+        // module.exports = function () { return document.querySelector("#foo").shadowRoot }
+        getElement = options.insertIntoCallable != null


var element = typeof options.insertInto === 'function' ? getElement(options.insertInto) : getElement(getTarget)

Another way maybe to pass the options to getTarget directly =>

var getTarget = function (target, options) { return typeof options.insertInto === 'function' ? options.insertInto() : function (target) { return document.querySelector(target); } }

Something like that

savelichalex · 2017-12-04T17:37:02Z

@michael-ciniawsky I can't reply directly to comment for puppeteer 😄 I really like this idea, but I don't think that this PR a good place for changing test runner :) what do you think?

michael-ciniawsky · 2017-12-04T19:40:25Z

@savelichalex This was just a suggestion not a requirement or the like :)

savelichalex · 2017-12-05T09:16:10Z

options.json

@@ -14,9 +14,6 @@
    "insertAt": {
      "type": ["string", "object"]
    },
-    "insertInto": {
-      "type": "string"


Looks like we need new keyword in ajv for this

Yep, a newer version of schema-utils is needed

michael-ciniawsky

This also requires updates to the docs (README) then 😛

michael-ciniawsky · 2017-12-06T06:34:22Z

index.js

@@ -35,6 +35,14 @@ module.exports.pitch = function (request) {
 		"}"
 	].join("\n");

+        var insertInto = "void 0";


Why void 0 ?

In this place https://github.com/savelichalex/style-loader/blob/fe58dbe66444852d9133a00d2d1883eb9036ff41/index.js#L55 I need some explicit value. This can be undefined or null of course)

Yep, just var insertInto; please :) . Also please move your code to L21+

michael-ciniawsky · 2017-12-06T06:37:50Z

index.js

+        if (typeof options.insertInto === "function") {
+          insertInto = options.insertInto.toString();
+        }
+        if (typeof options.insertInto === "string") {


This check shouldn't be necessary as there isn't anything to handle within the pitching phase of the loader in case options.insertInto is a {String}

I just want to make this check explicit, if you think that this is excess, I can delete this)

If it's not mandatory (I think it isn't, but didn't test it) please remove it 😛

michael-ciniawsky · 2017-12-06T06:38:05Z

index.js

@@ -35,6 +35,14 @@ module.exports.pitch = function (request) {
 		"}"
 	].join("\n");

+        var insertInto = "void 0";
+        if (typeof options.insertInto === "function") {


michael-ciniawsky · 2017-12-06T06:39:18Z

lib/addStyles.js

-	return function(selector) {
-		if (typeof memo[selector] === "undefined") {
-			var styleTarget = fn.call(this, selector);
+	return function(insertInto) {


insertInto => target

michael-ciniawsky · 2017-12-06T06:44:07Z

lib/addStyles.js

-			var styleTarget = fn.call(this, selector);
+	return function(insertInto) {
+                // If passing function in options, then use it for resolve "head" element.
+                // Usefull for Shadow Root style i.e


Usefull =>Useful

savelichalex · 2017-12-11T11:56:04Z

@michael-ciniawsky Sorry for late response

This also requires updates to the docs (README) then 😛

Agree :)

cthorner · 2017-12-11T16:37:21Z

Would be nice with the shadow dom / iframe targeting to be able to load the styles to multiple targets. Maybe outside of the scope of this PR?

Thanks for adding this functionality.

savelichalex · 2017-12-11T16:41:12Z

@cthorner Yeah, I think about it too! But unfortunately I don't know how to do it right. I'm also want to add styles not right after script evaluating. For me nice example of overriding runtime behavior is svg-sprite-loader

michael-ciniawsky

-       var insertInto = "void 0";
        if (typeof options.insertInto === "function") {
-          insertInto = options.insertInto.toString();
+          options.insertInto = options.insertInto.toString();
        }
-       if (typeof options.insertInto === "string") {
-          insertInto = '"' + options.insertInto + '"';
-        }

  "// Prepare cssTransformation",
  "var transform;",
  options.transform ? "transform = require(" + loaderUtils.stringifyRequest(this, "!" + path.resolve(options.transform)) + ");" : "",
- "var insertInto = " + insertInto + ";" ,
  "var options = " + JSON.stringify(options), // using toString() should be working here (?)
  "options.transform = transform",
- "options.insertInto = insertInto;",

And then we are g2g I guess :)

michael-ciniawsky · 2017-12-13T02:58:42Z

index.js

@@ -35,6 +35,14 @@ module.exports.pitch = function (request) {
 		"}"
 	].join("\n");

+        var insertInto = "void 0";


Yep, just var insertInto; please :) . Also please move your code to L21+

michael-ciniawsky · 2017-12-13T02:59:45Z

index.js

+        if (typeof options.insertInto === "function") {
+          insertInto = options.insertInto.toString();
+        }
+        if (typeof options.insertInto === "string") {


If it's not mandatory (I think it isn't, but didn't test it) please remove it 😛

michael-ciniawsky · 2017-12-13T03:02:34Z

index.js

@@ -44,8 +52,10 @@ module.exports.pitch = function (request) {
 		"// Prepare cssTransformation",
 		"var transform;",
 		options.transform ? "transform = require(" + loaderUtils.stringifyRequest(this, "!" + path.resolve(options.transform)) + ");" : "",
-		"var options = " + JSON.stringify(options),
-		"options.transform = transform",
+                "var insertInto = " + insertInto + ";" ,


This is redundant with the reassignment on L57 ? 🙃

savelichalex · 2017-12-13T17:21:14Z

@michael-ciniawsky I'll try to explain all this code :)
Why we need var insertInto and then assign to this variable instead of options? This is because if we just store function in options, then after JSON.stringify they quoted and in runtime will be just plain text nor function.
Why we need typeof options.insertInto === "string"? I found that without this check unspecified options become "undefined" and then in runtime throw an error, because of this.

michael-ciniawsky · 2018-01-08T07:56:55Z

@savelichalex Please rebase once you have the time, so I can finally finish this :)

alexander-akait requested review from alexander-akait, michael-ciniawsky and joshwiens December 4, 2017 13:36

alexander-akait added priority: 5 (nice to have) pr_action: Review semver: Minor type: Feature labels Dec 4, 2017

savelichalex commented Dec 4, 2017

View reviewed changes

savelichalex mentioned this pull request Dec 4, 2017

insertInto and ShadowDom #256

Closed

royroev approved these changes Dec 4, 2017

View reviewed changes

michael-ciniawsky added this to the 0.20.0 milestone Dec 4, 2017

michael-ciniawsky requested changes Dec 4, 2017

View reviewed changes

michael-ciniawsky changed the title ~~feat: Add the opportunity to use function to insertInto element~~ feat: support passing a {Function} (options.insertInto) Dec 5, 2017

savelichalex commented Dec 5, 2017

View reviewed changes

michael-ciniawsky requested changes Dec 6, 2017

View reviewed changes

michael-ciniawsky added the flag: Needs Docs label Dec 6, 2017

michael-ciniawsky requested changes Dec 13, 2017

View reviewed changes

michael-ciniawsky added priority: 4 (important) and removed flag: Needs Docs priority: 5 (nice to have) labels Dec 13, 2017

michael-ciniawsky changed the base branch from master to feature/insertInto January 8, 2018 07:55

michael-ciniawsky added the pr_action: Rebase label Jan 8, 2018

This was referenced Nov 7, 2019

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 Narshe1412/reactBPFoundationSCSS#7

Merged

[Snyk] Upgrade style-loader from 0.8.3 to 0.23.1 Smarsh/kendo-ui-webpack#1

Open

snyk-bot mentioned this pull request Jan 22, 2020

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 anyulled/Learn-Redux-Starter-Files#2

Open

This was referenced Feb 17, 2020

[Snyk] Upgrade style-loader from 0.18.2 to 0.23.1 DavidKindler/burger-builder#27

Merged

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 saurabharch/stitch-examples#1

Open

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 saurabharch/CodeGenius#3

Open

snyk-bot mentioned this pull request Mar 17, 2020

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 yowainwright/react-workshop#7

Merged

This was referenced May 4, 2020

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 Barnstorm-Online/votetracker#2

Open

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 React-Junkie/react-coffee-and-friends#3

Open

apiiro-snyk mentioned this pull request Jun 20, 2024

[Snyk] Upgrade style-loader from 0.16.1 to 0.23.1 apiiro-snyk/patient-browser#2

Open

KrisPeachyKen mentioned this pull request Jul 30, 2024

[Snyk] Upgrade style-loader from 0.12.4 to 0.23.1 KrisPeachyKen/ai#5

Open

superadmiral mentioned this pull request Jul 30, 2024

[Snyk] Upgrade style-loader from 0.12.4 to 0.23.1 superadmiral/ai#5

Open

YinghanFeng mentioned this pull request Aug 2, 2024

[Snyk] Upgrade style-loader from 0.8.3 to 0.23.1 YinghanFeng/deco-ide#2

Open

go-benchmark1 mentioned this pull request Aug 16, 2024

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 go-benchmark1/pome#2

Open

lucasaugustodeveloper mentioned this pull request Sep 8, 2024

[Snyk] Upgrade style-loader from 0.13.1 to 0.23.1 lucasaugustodeveloper/app-my-money#118

Open

Vishesh7797 mentioned this pull request Oct 26, 2024

[Snyk] Upgrade style-loader from 0.13.2 to 0.23.1 Vishesh7797/Sorting-Visualizer#1

Open

anmipa mentioned this pull request Nov 6, 2024

[Snyk] Upgrade style-loader from 0.12.4 to 0.23.1 City-of-Helsinki/servicemap-embedder#2

Open

feat: support passing a {Function} (options.insertInto) #279

feat: support passing a {Function} (options.insertInto) #279

Conversation

savelichalex commented Dec 4, 2017

jsf-clabot commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky left a 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

savelichalex Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

savelichalex Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

michael-ciniawsky Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

savelichalex commented Dec 4, 2017

michael-ciniawsky commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Dec 13, 2017 • 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

savelichalex commented Dec 11, 2017 • edited Loading

cthorner commented Dec 11, 2017

savelichalex commented Dec 11, 2017

michael-ciniawsky left a comment • edited Loading

Choose a reason for hiding this comment

michael-ciniawsky Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

michael-ciniawsky Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savelichalex commented Dec 13, 2017

michael-ciniawsky commented Jan 8, 2018 • edited Loading

feat: support passing a `{Function}` (`options.insertInto`) #279

feat: support passing a `{Function}` (`options.insertInto`) #279

jsf-clabot commented Dec 4, 2017 •

edited

Loading

savelichalex Dec 4, 2017 •

edited

Loading

savelichalex Dec 4, 2017 •

edited

Loading

michael-ciniawsky Dec 4, 2017 •

edited

Loading

michael-ciniawsky Dec 4, 2017 •

edited

Loading

michael-ciniawsky commented Dec 4, 2017 •

edited

Loading

michael-ciniawsky Dec 13, 2017 •

edited

Loading

michael-ciniawsky Dec 13, 2017 •

edited

Loading

savelichalex commented Dec 11, 2017 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading

michael-ciniawsky Dec 13, 2017 •

edited

Loading

michael-ciniawsky Dec 13, 2017 •

edited

Loading

michael-ciniawsky commented Jan 8, 2018 •

edited

Loading