Skip to content

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

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

Merged
merged 8 commits into from
Jun 12, 2017
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
933085d
insertInto for iframes
Jun 6, 2017
e786fb9
fix unit test
Jun 6, 2017
685e503
fix typo
Jun 6, 2017
196641e
revert whitespace changes
Jun 6, 2017
f8a8582
convert space indent to tabs (like rest of document)
Jun 6, 2017
1b788f2
use try/catch only if element is a iframe
Jun 6, 2017
6b8dec5
Revert "convert space indent to tabs (like rest of document)"
Jun 6, 2017
b998280
readme improvement: use link for "IFrame"
Jun 12, 2017
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
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 12, 2017

Choose a reason for hiding this comment

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

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

You can also insert the styles into a [ShadowRoot](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot), e.g

**webpack.config.js**
```js
Expand Down
40 changes: 25 additions & 15 deletions lib/addStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

@bebraw bebraw Jun 6, 2017

Choose a reason for hiding this comment

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

/s/insetead/instead

try {
if (styleTarget instanceof window.HTMLIFrameElement) {
Copy link
Member

@joshwiens joshwiens Jun 6, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@tobias-zucali tobias-zucali Jun 6, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@joshwiens joshwiens Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 12, 2017

Choose a reason for hiding this comment

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

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 will throw an exception if access to iframe is blocked
// due to cross-origin restrictions
styleTarget = styleTarget.contentDocument.head;
}
} catch(e) {
styleTarget = null;
}
memo[selector] = styleTarget;
}

return memo[selector]
};
})(function (target) {
Expand Down Expand Up @@ -209,19 +219,19 @@ function addStyle (obj, options) {

// If a transform function was defined, run it on the css
if (options.transform && obj.css) {
result = options.transform(obj.css);

if (result) {
// If transform returns a value, use that instead of the original css.
// This allows running runtime transformations on the css.
obj.css = result;
} else {
// If the transform function returns a falsy value, don't add this css.
// This allows conditional loading of css
return function() {
// noop
};
}
result = options.transform(obj.css);
Copy link

@bebraw bebraw Jun 6, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@tobias-zucali tobias-zucali Jun 6, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 6, 2017

Choose a reason for hiding this comment

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

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.


if (result) {
// If transform returns a value, use that instead of the original css.
// This allows running runtime transformations on the css.
obj.css = result;
} else {
// If the transform function returns a falsy value, don't add this css.
// This allows conditional loading of css
return function() {
// noop
};
}
}

if (options.singleton) {
Expand Down
12 changes: 12 additions & 0 deletions test/basicTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ describe("basic tests", function() {
"<div class='target'>",
checkValue,
"</div>",
"<iframe class='iframeTarget'/>",
"</body>",
"</html>"
].join("\n");
Expand Down Expand Up @@ -103,6 +104,17 @@ describe("basic tests", function() {
runCompilerTest(expected, done, undefined, selector);
}); // it insert into

it("insert into iframe", function(done) {
let selector = "iframe.iframeTarget";
styleLoaderOptions.insertInto = selector;

let expected = requiredStyle;

runCompilerTest(expected, done, function() {
return this.document.querySelector(selector).contentDocument.head.innerHTML;
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 6, 2017

Choose a reason for hiding this comment

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

Why this.document and not just document ?

Copy link
Author

@tobias-zucali tobias-zucali Jun 6, 2017

Choose a reason for hiding this comment

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

True, I can improve that ;-)

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 6, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@tobias-zucali tobias-zucali Jun 6, 2017

Choose a reason for hiding this comment

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

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.

}, selector);
}); // it insert into

it("singleton", function(done) {
// Setup
styleLoaderOptions.singleton = true;
Expand Down