Skip to content
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

feat: support 'before' insertions (options.insertAt) #253

Merged
merged 7 commits into from
Sep 8, 2017
Merged

feat: support 'before' insertions (options.insertAt) #253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d8e641
Add insertAt='before' option
iancw Jun 27, 2017
0bee7a6
Improve browser error message
iancw Jun 27, 2017
03c8170
Remove insertBefore option in favor of an object
iancw Jun 27, 2017
5618dd2
Slim down the new Object options
iancw Jun 27, 2017
0caab23
update readme with slimmer object syntax
iancw Jun 27, 2017
55d2e41
Improve insertAt before README sections
iancw Jun 27, 2017
934bbaf
Remove old inline example from README
iancw Jun 27, 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
14 changes: 14 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ Styles are not added on `import/require()`, but instead on call to `use`/`ref`.
|**`transform`** |`{Function}`|`false`|Transform/Conditionally load CSS by passing a transform/condition function|
|**`insertAt`**|`{String}`|`bottom`|Inserts `<style></style>` at the given position|
|**`insertInto`**|`{String}`|`<head>`|Inserts `<style></style>` into the given position|
|**`insertBefore`**|`{String}`|``|A query string specifying an element to insert new `<style>` before, required when `insertAt="before"`|
Copy link
Member

Choose a reason for hiding this comment

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

I disagree with an extra top level option depending on other one. Possible to pass the an {Object} to insertAt instead { type: 'before', $: '#id' } ($ maybe better named selector) [Needs discussion]

Copy link
Contributor Author

@iancw iancw Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I wasn't completely sure the best way to approach this. The logic for insertBefore could also be triggered without requiring anything special in insertAt. In other words, if insertBefore has a non-empty string that behavior pre-empts the normal insertion logic triggered by insertAt.

I steered away from that because it seemed potentially confusing to have insertBefore pre-empt and essentially override the behavior of insertAt.

I'm also fine with making insertAt an object, but that becomes a breaking change. If you think that's best I can go with it, but it seems kind of heavy-handed for a small feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see how it can be done in a backwards-compatible fashion. Pushed an implementation. Another option for the Object interface is { before: '#id'}, which lets us avoid the ambiguous-sounding type key and avoid choosing between $ and selector.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah an {Object} isn't optimal in terms of type consistency either, but inter option dependence isn't aswell, the interface you suggested is much better though :)

|**`sourceMap`**|`{Boolean}`|`false`|Enable/Disable Sourcemaps|
|**`convertToAbsoluteUrls`**|`{Boolean}`|`false`|Coverts relative URLs to absolute urls, when source maps are enabled|

Expand Down Expand Up @@ -285,6 +286,19 @@ By default, the style-loader appends `<style>` elements to the end of the style
}
```

A new `<style>` element can be inserted before a specific element using the `before` value

**webpack.config.js**
```js
{
loader: 'style-loader'
options: {
insertAt: 'before',
insertBefore: '#style-overrides'
}
}
```

### `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 you can specify a CSS selector for that element here. If you target an [IFrame](https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement) make sure you have sufficient access rights, the styles will be injected into the content document head.
You can also insert the styles into a [ShadowRoot](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot), e.g
Expand Down
5 changes: 5 additions & 0 deletions lib/addStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ module.exports = function(list, options) {
// By default, add <style> tags to the bottom of the target
if (!options.insertAt) options.insertAt = "bottom";

if (options.insertAt === "before" && !options.insertBefore) throw new Error("Must specify selector in `insertBefore` when specifying `insertAt = 'before'`");

var styles = listToStyles(list, options);

addStylesToDom(styles, options);
Expand Down Expand Up @@ -170,6 +172,9 @@ function insertStyleElement (options, style) {
stylesInsertedAtTop.push(style);
} else if (options.insertAt === "bottom") {
target.appendChild(style);
} else if (options.insertAt === "before") {
var nextSibling = getElement(options.insertInto + " " + options.insertBefore);
target.insertBefore(style, nextSibling);
} else {
throw new Error("Invalid value for parameter 'insertAt'. Must be 'top' or 'bottom'.");
Copy link

Choose a reason for hiding this comment

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

Error message should include valid option 'before':

Invalid value for parameter 'insertAt'. Must be 'top', 'bottom' or 'before'.

Copy link
Member

Choose a reason for hiding this comment

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

Will this throw in the browser console or during build time? If the latter

this.emitError("Style Loader\n\n Invalid value for parameter 'insertAt' ('options.insertAt') found.\n Must be 'top' or 'bottom'.\n (https://github.com/webpack-contrib/style-loader#insertat)\n")

else

throw new Error("[Style Loader]\n\n Invalid value for parameter 'insertAt' ('options.insertAt') found.\n Must be 'top' or 'bottom'.\n (https://github.com/webpack-contrib/style-loader#insertat)\n")

}
Expand Down
20 changes: 19 additions & 1 deletion test/basicTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe("basic tests", function() {
}
`,
requiredStyle = `<style type="text/css">${requiredCss}</style>`,
existingStyle = "<style>.existing { color: yellow }</style>",
existingStyle = `<style id="existing-style">.existing { color: yellow }</style>`,
checkValue = '<div class="check">check</div>',
rootDir = path.resolve(__dirname + "/../") + "/",
jsdomHtml = [
Expand Down Expand Up @@ -95,6 +95,24 @@ describe("basic tests", function() {
runCompilerTest(expected, done);
}); // it insert at top

it("insert at before", function(done) {
styleLoaderOptions.insertAt = "before";
styleLoaderOptions.insertBefore = "#existing-style";

let expected = [requiredStyle, existingStyle].join("");

runCompilerTest(expected, done);
}); // it insert at before

it("insert at before invalid selector", function(done) {
styleLoaderOptions.insertAt = "before";
styleLoaderOptions.insertBefore = "#missing";

let expected = [existingStyle, requiredStyle].join("\n");

runCompilerTest(expected, done);
}); // it insert at before

it("insert into", function(done) {
let selector = "div.target";
styleLoaderOptions.insertInto = selector;
Expand Down