feat: add support for iframes (`options.insertInto`) #248

tobias-zucali · 2017-06-06T06:29:16Z

What kind of change does this PR introduce?

add possibility to target an iframe with the insertInto setting
select head in the iframes content document
use try/catch to avoid breakting the code if permission is denied

Did you add tests for your changes?
Yes.

If relevant, did you update the README?
Yes.

Summary
Pull request for open issue insertInto for iframes #213

Does this PR introduce a breaking change?

No.

Other information

Closes #213

- add possibility to target an iframe with the `insertInto` setting - select head in the iframes content document - use try/catch to avoid breakting the code if permission is denied - add unit test - extend documentation

jsf-clabot · 2017-06-06T06:29:17Z

All committers have signed the CLA.

michael-ciniawsky · 2017-06-06T06:35:13Z

test/basicTest.js

+    let expected = 'requiredStyle ';
+
+    runCompilerTest(expected, done, function() {
+      return this.document.querySelector(selector).contentDocument.head.innerHTML;


Why this.document and not just document ?

True, I can improve that ;-)

If it works without the this prefix, better ditch it :)

this is needed. Seems that the test is run in node or so… but scope is explicitly set to window, that's why this works.

bebraw · 2017-06-06T06:42:55Z

lib/addStyles.js

@@ -28,9 +28,19 @@ var getElement = (function (fn) {

 	return function(selector) {
 		if (typeof memo[selector] === "undefined") {
-			memo[selector] = fn.call(this, selector);
+			var styleTarget = fn.call(this, selector);
+			// Special case to return head of iframe insetead of iframe itself


/s/insetead/instead

bebraw · 2017-06-06T06:43:36Z

lib/addStyles.js

-	    		// noop
-	    	};
-	    }
+			result = options.transform(obj.css);


I'm not sure if this formatting fix is needed. Any thoughts?

Was working too fast ;-) was mixed blanks with tabs, but the automatic conversion choose the wrong indent level. Should I fix the indentation level or revert it to mixed blanks/tabs?

I though it is github display madness with tabs tbh (I will send a style PR converting to spaces 😛).
But in case it's not @tobias-zucali please use the indent the rest of the code currently uses.

michael-ciniawsky · 2017-06-06T07:06:45Z

Fixes #213

joshwiens · 2017-06-06T10:30:03Z

lib/addStyles.js

+			return function() {
+				// noop
+			};
+		}


Can you shift this block back, no reason for this to show up in the diff as there are not code changes less what looks like an auto-format.

joshwiens · 2017-06-06T10:33:38Z

lib/addStyles.js

+			var styleTarget = fn.call(this, selector);
+			// Special case to return head of iframe instead of iframe itself
+			try {
+				if (styleTarget instanceof window.HTMLIFrameElement) {


Do it not make more sense to check instanceof first? As it stands right now, we are going to execute that try/catch block every time regardless. When in reality, it only needs to execute if styleTarget instanceof window.HTMLIFrameElement

I was not sure in which environments style-loader can be run … is it possible that window.HTMLIFrameElement does not exist there?

My point was more that a truthy check is faster than executing a try catch iirc. Given the style loader chain already suffers from performance issues at scale, we need to be efficient when and wherever possible.

//cc @bebraw @michael-ciniawsky

If there are concerns about different execution contexts, we could check window along with that or in a higher scope and it should still be faster. Though historically, our stance on style-loader is that it's intended to load styles into the DOM & to use something like extract-text when you are outside the browser.

Yep, style-loader should only support environments which expose/mimic a DOM Interface, the tests already use JSDOM which is the primary non-browser target style-loader supports atm. @tobias-zucali Could you try without the try/catch and instanceof check and see if it works ? Maybe add a comment with the current solution in case it heavily breaks other people's setups and we need a hotfix :). Otherwise good to go imho

This reverts commit f8a8582.

michael-ciniawsky · 2017-06-12T07:28:19Z

README.md

@@ -286,7 +286,8 @@ By default, the style-loader appends `<style>` elements to the end of the style
 ```

 ### `insertInto`
-By default, the style-loader inserts the `<style>` elements into the `<head>` tag of the page. If you want the tags to be inserted somewhere else, e.g. into a [ShadowRoot](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot), you can specify a CSS selector for that element here, e.g
+By default, the style-loader inserts the `<style>` elements into the `<head>` tag of the page. If you want the tags to be inserted somewhere else you can specify a CSS selector for that element here. If you target an iframe make sure you have sufficient access rights, the styles will be injected into the content document head.


iframe => [IFrame](https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement)

michael-ciniawsky

@tobias-zucali Thx

Kavenon · 2017-07-17T22:54:31Z

When will this feature be published in npm repo?

insertInto for iframes

933085d

- add possibility to target an iframe with the `insertInto` setting - select head in the iframes content document - use try/catch to avoid breakting the code if permission is denied - add unit test - extend documentation

fix unit test

e786fb9

michael-ciniawsky reviewed Jun 6, 2017

View reviewed changes

michael-ciniawsky changed the title ~~insertInto for iframes~~ feat: add support for iframes (options.insertInto) Jun 6, 2017

michael-ciniawsky added priority: 4 (important) pr_action: Review semver: Minor type: Feature labels Jun 6, 2017

michael-ciniawsky self-assigned this Jun 6, 2017

michael-ciniawsky added this to the 0.19.0 milestone Jun 6, 2017

michael-ciniawsky requested review from bebraw, alexander-akait and joshwiens June 6, 2017 06:39

bebraw reviewed Jun 6, 2017

View reviewed changes

Tobias Zucali added 3 commits June 6, 2017 08:49

fix typo

685e503

revert whitespace changes

196641e

convert space indent to tabs (like rest of document)

f8a8582

michael-ciniawsky approved these changes Jun 6, 2017

View reviewed changes

joshwiens reviewed Jun 6, 2017

View reviewed changes

Tobias Zucali added 2 commits June 6, 2017 12:57

use try/catch only if element is a iframe

1b788f2

Revert "convert space indent to tabs (like rest of document)"

6b8dec5

This reverts commit f8a8582.

michael-ciniawsky reviewed Jun 12, 2017

View reviewed changes

michael-ciniawsky approved these changes Jun 12, 2017

View reviewed changes

readme improvement: use link for "IFrame"

b998280

joshwiens added pr_action: Merge ready and removed pr_action: Review labels Jun 12, 2017

joshwiens approved these changes Jun 12, 2017

View reviewed changes

michael-ciniawsky approved these changes Jun 12, 2017

View reviewed changes

michael-ciniawsky merged commit 25e8e89 into webpack-contrib:master Jun 12, 2017

michael-ciniawsky removed this from the 0.20.0 milestone Nov 11, 2017

renovate bot mentioned this pull request Mar 22, 2018

Update style-loader to ^0.21.0 sbardian/DebtTrackerAPI#30

Merged

shanecav mentioned this pull request Jun 9, 2018

update style-loader to 0.21.0 preactjs/preact-cli#585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for iframes (`options.insertInto`) #248

feat: add support for iframes (`options.insertInto`) #248

tobias-zucali commented Jun 6, 2017 •

edited by joshwiens

Loading

jsf-clabot commented Jun 6, 2017 •

edited

Loading

michael-ciniawsky Jun 6, 2017

tobias-zucali Jun 6, 2017

michael-ciniawsky Jun 6, 2017

tobias-zucali Jun 6, 2017

bebraw Jun 6, 2017

bebraw Jun 6, 2017

tobias-zucali Jun 6, 2017

michael-ciniawsky Jun 6, 2017

michael-ciniawsky commented Jun 6, 2017 •

edited

Loading

joshwiens Jun 6, 2017

joshwiens Jun 6, 2017

tobias-zucali Jun 6, 2017

joshwiens Jun 6, 2017 •

edited

Loading

michael-ciniawsky Jun 12, 2017

michael-ciniawsky Jun 12, 2017

michael-ciniawsky left a comment

Kavenon commented Jul 17, 2017

feat: add support for iframes (options.insertInto) #248

feat: add support for iframes (options.insertInto) #248

Conversation

tobias-zucali commented Jun 6, 2017 • edited by joshwiens Loading

jsf-clabot commented Jun 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwiens Jun 6, 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 left a comment

Choose a reason for hiding this comment

Kavenon commented Jul 17, 2017

feat: add support for iframes (`options.insertInto`) #248

feat: add support for iframes (`options.insertInto`) #248

tobias-zucali commented Jun 6, 2017 •

edited by joshwiens

Loading

jsf-clabot commented Jun 6, 2017 •

edited

Loading

michael-ciniawsky commented Jun 6, 2017 •

edited

Loading

joshwiens Jun 6, 2017 •

edited

Loading