feat: support 'before' insertions (options.insertAt)
#253
Conversation
This allows styles to be inserted before other styles or links that exist on the page. This is necessary to allow ordering style overrides between multiple sources of styles.
|
|
michael-ciniawsky
changed the title
options.insertBefore)
michael-ciniawsky
requested review from
bebraw,
alexander-akait,
michael-ciniawsky and
joshwiens
lib/addStyles.js
Outdated
| @@ -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'."); | |||
There was a problem hiding this comment.
Error message should include valid option 'before':
Invalid value for parameter 'insertAt'. Must be 'top', 'bottom' or 'before'.
There was a problem hiding this comment.
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")
lib/addStyles.js
Outdated
| @@ -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'."); | |||
There was a problem hiding this comment.
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")
README.md
Outdated
| @@ -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"`| | |||
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
michael-ciniawsky
changed the title
options.insertBefore)options.insertAt)
README.md
Outdated
| loader: 'style-loader' | ||
| options: { | ||
| insertAt: { | ||
| before: '#style-overrides' |
There was a problem hiding this comment.
#id || #name (generic)
There was a problem hiding this comment.
The options table needs an type update aswell {String} => {String\|Object} (\ [escaped])
|
@webpack-contrib/org-maintainers please review :) |
michael-ciniawsky
added
pr_action: Review
status: Approved
and removed
pr_action: Discuss
status: Needs triage
labels
Allow new styles to be inserted before other
<style>or<link>elements thatexist on the page. This allows more precise ordering for complex style overrides
between multiple sources.
What kind of change does this PR introduce?
New feature
Did you add tests for your changes?
Yes
If relevant, did you update the README?
Yes
Summary
This is a new feature adding a third possible value for
insertAt:"before". A new option valueinsertBeforewhich contains query selector to select the element that the new styles should precede.Complicated style situations where multiple
<style>tags exist and must be inserted in specific order to allow style-overrides to work across sections require more precise placement than justtoporbottom. This feature allows more precise selection of exactly where to insert a new<style>section relative to other existing sections.Does this PR introduce a breaking change?
No