feat: add hook for customizing injected runtime tags #526
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| /* eslint-disable class-methods-use-this */ | ||
|
|
||
| import { validate } from 'schema-utils'; | ||
| import { SyncWaterfallHook } from 'tapable'; | ||
|
|
||
| import schema from './plugin-options.json'; | ||
| import { MODULE_TYPE, compareModulesByIdentifier } from './utils'; | ||
|
|
@@ -28,6 +29,8 @@ const cssModuleCache = new WeakMap(); | |
| */ | ||
| const cssDependencyCache = new WeakMap(); | ||
|
|
||
| const compilerHookMap = new WeakMap(); | ||
|
|
||
| class MiniCssExtractPlugin { | ||
| static getCssModule(webpack) { | ||
| /** | ||
|
|
@@ -300,6 +303,20 @@ class MiniCssExtractPlugin { | |
| return CssDependency; | ||
| } | ||
|
|
||
| static getCompilerHooks(compiler) { | ||
| /** | ||
| * Prevent creation of multiple compiler hook maps to allow other integrations to get the current mapping. | ||
| */ | ||
| let hooks = compilerHookMap.get(compiler); | ||
| if (!hooks) { | ||
| hooks = { | ||
| customize: new SyncWaterfallHook(['attributes']), | ||
| }; | ||
| compilerHookMap.set(compiler, hooks); | ||
| } | ||
| return hooks; | ||
| } | ||
|
|
||
| constructor(options = {}) { | ||
| validate(schema, options, { | ||
| name: 'Mini CSS Extract Plugin', | ||
|
|
@@ -847,30 +864,46 @@ class MiniCssExtractPlugin { | |
| return null; | ||
| } | ||
|
|
||
| const attributes = { | ||
| href: `${RuntimeGlobals.publicPath} + ${RuntimeGlobals.require}.miniCssF(chunkId)`, | ||
| rel: JSON.stringify('stylesheet'), | ||
| onload: 'onLinkComplete', | ||
| onerror: 'onLinkComplete', | ||
| }; | ||
|
|
||
| // Some attributes cannot be assigned through setAttribute, so we maintain | ||
| // a list of attributes that can safely be assigned through dot notation | ||
| const safeAttrs = ['href', 'rel', 'type', 'onload', 'onerror']; | ||
|
There was a problem hiding this comment. I think we should change this logic little bit, I think we don't protect attributes, developer can add There was a problem hiding this comment. The reason for this logic is that some attributes must be assigned through I'm not aware of any sensible way of determining what attributes needs to be set which way, so one proposal here would be to rewrite the above code to say Perhaps we could also update the wording and the comment to clarify why this is done. The problem with that solution is that user-defined attributes (other than onload and onerror) that must be assigned through dot notation will not work with that solution. What's your thoughts on this? There was a problem hiding this comment. Hi @alexander-akait , There was a problem hiding this comment. @utlime need rebase, I think it is stale PR, so feel free to send this code again and we will merge it There was a problem hiding this comment. @alexander-akait Not at all, I have been waiting for your response to my latest comment There was a problem hiding this comment. I think we can write warning in docs about some attributes, they are still internal and should not change, it will simplify logic |
||
|
|
||
| if (this.runtimeOptions.linkType) { | ||
| attributes.type = JSON.stringify(this.runtimeOptions.linkType); | ||
| } | ||
|
|
||
| if (crossOriginLoading) { | ||
| attributes.crossOrigin = `(linkTag.href.indexOf(window.location.origin + '/') !== 0) | ||
| ? ${JSON.stringify(crossOriginLoading)} | ||
| : undefined`; | ||
| } | ||
|
|
||
| // Append static attributes | ||
| if (this.runtimeOptions.attributes) { | ||
| Object.entries(this.runtimeOptions.attributes).forEach( | ||
| ([key, value]) => { | ||
| attributes[key] = JSON.stringify(value); | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| // Append dynamic attributes | ||
| MiniCssExtractPlugin.getCompilerHooks(compiler).customize.call( | ||
|
There was a problem hiding this comment. Also let's rename |
||
| attributes | ||
| ); | ||
|
|
||
| return Template.asString([ | ||
| `var createStylesheet = ${runtimeTemplate.basicFunction( | ||
| 'chunkId, fullhref, resolve, reject', | ||
| [ | ||
| 'var linkTag = document.createElement("link");', | ||
| `var onLinkComplete = ${runtimeTemplate.basicFunction( | ||
| 'event', | ||
| [ | ||
|
|
@@ -892,19 +925,17 @@ class MiniCssExtractPlugin { | |
| '}', | ||
| ] | ||
| )}`, | ||
| Template.asString( | ||
| Object.entries(attributes).map(([key, value]) => { | ||
| if (safeAttrs.includes(key)) { | ||
| return `linkTag.${key} = ${value};`; | ||
| } | ||
|
|
||
| return `linkTag.setAttribute(${JSON.stringify( | ||
| key | ||
| )}, ${value});`; | ||
| }) | ||
| ), | ||
| typeof this.runtimeOptions.insert !== 'undefined' | ||
| ? typeof this.runtimeOptions.insert === 'function' | ||
| ? `(${this.runtimeOptions.insert.toString()})(linkTag)` | ||
|
|
@@ -945,7 +976,7 @@ class MiniCssExtractPlugin { | |
| 'resolve, reject', | ||
| [ | ||
| `var href = ${RuntimeGlobals.require}.miniCssF(chunkId);`, | ||
| `var fullhref = ${attributes.href};`, | ||
| 'if(findStylesheet(href, fullhref)) return resolve();', | ||
| 'createStylesheet(chunkId, fullhref, resolve, reject);', | ||
| ] | ||
|
|
@@ -1016,7 +1047,7 @@ class MiniCssExtractPlugin { | |
| 'chunkId', | ||
| [ | ||
| `var href = ${RuntimeGlobals.require}.miniCssF(chunkId);`, | ||
| `var fullhref = ${attributes.href};`, | ||
| 'var oldTag = findStylesheet(href, fullhref);', | ||
| 'if(!oldTag) return;', | ||
| `promises.push(new Promise(${runtimeTemplate.basicFunction( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-akait I started rebasing and changing the code based on your comments. Are you suggesting that we remove all properties (i.e. attributes that cannot be set through setAttribute) from the above attributes object?
Otherwise I suggest that we change the below declaration to
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think docs about these attributes will be enough, they are internal and rewriting them is not safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so keep the above behavior but add section to docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-akait Hi there, any plans on pulling this in soon? I followed the discussion and it seemed like you were almost done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
解决了吗？目前也是这个sri问题，需要这个功能