feat: add hook for customizing injected runtime tags #526

patriksletmo · 2020-04-24T08:34:03Z

This PR contains a:

Motivation / Use-Case

This contribution includes a hook that can be used to customize the attributes link tags for lazy loaded CSS chunks, and was inspired by #40. As stated in this issue, the new functionality allow plugins such as webpack-subresource-integrity to work properly with injected tags.

Example usage from an external plugin would look something like this:

var MiniCssExtractPlugin = require("mini-css-extract-plugin");

const compiler; // Replace with Webpack compiler instance
const cssHooks = MiniCssExtractPlugin.getCompilerHooks(compiler);
if (cssHooks) {
  cssHooks.customize.tap('MyPlugin', (source) => source + "\n" + "linkTag.integrity = myIntegrityMap[chunkId];");
}

Breaking Changes

None.

Additional Info

The added functionality is very simple, and gives a greater freedom to other plugins wishing to utilize this functionality. However, this also means that this contribution is quite small. Please let me know if there is anything that can be improved and I will do my best to implement changes in a timely manner.

jscheid · 2020-04-24T20:00:06Z

@patriksletmo many thanks for taking a stab at this. I like how simple this change is, but compared to the generic approach I've outlined in #40 I'm afraid it has a few drawbacks from the perspective of our plugin (webpack-subresource-integrity):

It would require us to get tied to a number of implementation details, such as the variable name linkTag, so there's no (implicit) guarantee it would keep working with future versions of mini-css-extract-plugin. If MCEP were to change their implementation in the future we would then need to start probing its package version and use different code paths depending.
We'd need to declare a peer dependency on mini-css-extract-plugin, as we would need to require it and doing so without a declared dependency breaks with PnP. This is less of an issue now that Yarn supports optional dependencies but still not ideal, since npm doesn't and prints a warning for unmet peer dependencies.
Unlike the generic mechanism, this one couldn't be reused which means we would have to keep adding tailored support for any other (future) similar plugins, and would lose the opportunity to simplify our existing integration with html-webpack-plugin.

With all this in mind, I still think a more generic solution would be preferable. The main issue that would need to be solved first is to figure out where to declare the hook, and I think the best place for it is in Webpack itself, probably on the compilation object. I'm pretty sure I've proposed this a while back on Gitter and @sokra was open to the idea, so maybe it wouldn't take much to put this in place?

patriksletmo · 2020-04-26T09:34:17Z

@jscheid Thank you for your feedback!

I have thought about those drawbacks as well, and this is the motivation for going with the solution I've chosen:

I quite like the approach you suggested in the original issue, but I was unsure how to interpret what @michael-ciniawsky said about adding a "generic hook to add code to the JS side of affairs". I think that allowing other plugins to add code is more flexible, but I agree that it has the drawback of worse maintainability in case the internal code changes.
I am not too happy with this consequence, but I noticed that the hooks property on the compiler and compilation object will be frozen as of Webpack 5 (see more details here). The proposed implementation works regardless of this change, and should be compatible with Webpack 1 to 5.
See 1.

I agree that this change could probably be more useful with the generic implementation of your general proposal, but I think that the exposure of the hook should remain as is. If a more flexible solution is required in some cases, it might be better to have another hook for that purpose, but perhaps this can be implemented at a later time if needed. I'll update the code accordingly and submit an update to this PR.

jscheid · 2020-04-26T10:31:33Z

@patriksletmo I see. Thanks for pointing out the compilation hooks getting frozen, I wasn't aware of this.

Just thinking out loud, one solution could be to create a new, separate npm package -- something like webpack-injected-tag-hook -- whose sole purpose would be to expose the hook in question and to document the interface contract. This could be used by WSI, MCE and any other/future similar plugins as a point of rendezvous. How would everyone feel about this?

As for @michael-ciniawsky's comment about hot reloading, I'm also curious to hear what the requirements are, if we can kill two birds with one stone then I'm all for it. Perhaps he can add some more detail. In the meantime I'd suggest we focus on just the interface between WSI and MCE, which I understand is the problem you're trying to solve, in the interest of keeping the scope manageable.

patriksletmo · 2020-04-26T15:07:45Z

I am very positive to a central solution that connects different packages of this kind. It would certainly make the interface between them easier and more uniform, and has the added benefit of not creating unnecessary dependencies.

How would the interface for this solution look like? I suppose that the solution you proposed in #40 should hold here as well, by passing a data structure on the following form to all subscribers of the hook. I don't think subscribers would need more specificity than this, but perhaps you have a better understanding of this situation.

interface Tag {
    tagName: string,
    attributes: object
}

jscheid · 2020-04-26T20:54:29Z

That should work, I can't think of anything else it would need.

The next step would then probably be finding out if the maintainers of this plugin (MCE) are happy to add that dependency.

There is one potential pitfall with this approach, which is that it requires all plugins to depend on the same version of the package or they will get different hook instances back, similar to this issue. The package description should ask dependees to specify a loose version range like ^1.0.0. (That's a good idea anyway of course, but can't hurt to point it out.)

jscheid · 2020-04-26T21:32:29Z

Actually, thinking more about it, I have a doubt... webpack itself is injecting tags (for dynamic import/require.ensure) and for as long as it isn't using that hook, those will always need special treatment in our plugin. But if it is using that hook then adding it to webpack itself would be the best place. Before we go down the path with a separate package, perhaps we should see if they would be open to adding it after all? I assume freezing the hooks doesn't mean that they will never add anything more to it themselves, just that they don't want third-party plugins to add their own custom hooks.

patriksletmo · 2020-04-27T10:04:05Z

Yeah, it would be interesting to hear a maintainer's take on this.

To the best of my understanding, freezing the hooks object means that the hooks object can only be assigned, but never modified. In essence, the following should hold:

// Allowed
constructor() {
  this.hooks = {...}
}

// Disallowed
Module.hooks.myHook = ...

As for the situation with multiple different versions of the same package I think that there is a fairly reasonable solution to that problem. It involves using the global nodejs object to store the hook map. The object could then be declared something like this:

global.customPrefixCompilerHookMap = global.customPrefixCompilerHookMap || new WeakMap();

It should be noted that this solution needs consideration to the following limitations:

The global variable name must be unique
The map must never have a hook overridden
The map must have new hooks appended to the existing hook set to ensure that both old and new hooks are included when multiple versions of the common package exist within the project

jscheid · 2020-04-28T06:38:51Z

To the best of my understanding, freezing the hooks object means that the hooks object can only be assigned, but never modified.

I meant bake it directly into Webpack, but never mind. I think the separate hook package is a good compromise between clean and workable.

global.customPrefixCompilerHookMap

That could get messy depending on how exactly two released versions of the package will differ. I think just asking people not to depend on too narrow a version range, and making an effort not to release a new package version too often (if at all) should do the trick. It's going to be an extremely simple package so hopefully there will rarely be the need for a new version, and hopefully never for one with a breaking change.

jscheid · 2020-04-28T06:42:07Z

@jantimon would be great to hear your thoughts on this as well.

utlime · 2021-03-24T10:55:18Z

Hi @jahed @patriksletmo,
seems you already done some stuff around it. Why did you stop? =)

patriksletmo · 2021-03-30T08:01:20Z

Hey @utlime, no maintainer of this repository seemed interested in the idea so we didn't pursue this issue further.

alexander-akait · 2021-03-31T11:38:16Z

@patriksletmo Can you describe case? You can use attributes for it

patriksletmo · 2021-03-31T12:42:08Z

@alexander-akait With an implementation using attributes instead, a plugin using the hook could look something like this:

var MiniCssExtractPlugin = require("mini-css-extract-plugin");

const cssHooks = MiniCssExtractPlugin.getCompilerHooks(this.compilation.compiler);
if (cssHooks) {
  cssHooks.customize.tap("MyPlugin", data => {
    if (data.tagName === "link") {
      const integrityHash = getIntegrityForResource(data); // implementation specific definition
      data.attributes.integrity = JSON.stringify(integrityHash);
    }
  });
}

By requiring the hook consumer to provide the required value escaping we make it possible to have plugins interact with other parts of the script, for instance:

cssHooks.customize.tap("MyPlugin", data => {
  if (data.tagName === "link") {
    data.attributes.integrity = "window.integrityData[chunkId]";
  }
});

I'm not certain if this last scenario makes perfect sense since it allows for usage of implementation-specific variables that may change without notice, but on the other hand it opens up for more advanced implementations.

alexander-akait · 2021-03-31T12:49:21Z

@patriksletmo make sense, can you rebase?

patriksletmo · 2021-04-12T12:46:47Z

@alexander-akait Other work got in the way but I've updated the code and rebased against master now. I updated the tests for webpack 5 but couldn't get the tests for webpack 4 to run. Please let me know if you want me to add more tests for the added functionality.

alexander-akait · 2021-04-12T13:04:35Z

Let's skip test for v4 (we will drop it in future) and enable this only for webpack v5

Enable other plugins to tap into the tag injection mechanism in order to customize the functionality of lazy loaded chunks. Closes webpack-contrib#40.

patriksletmo · 2021-04-14T09:46:59Z

Alright, I've updated the code so that this feature is not used for webpack v4.

alexander-akait · 2021-04-14T10:40:33Z

src/index.js

+
+            // 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'];


I think we should change this logic little bit, I think we don't protect attributes, developer can add data-* attributes, any non valid HTML attribute (why not 😄 ), even onload and onerror can be rewritten

The reason for this logic is that some attributes must be assigned through tag.prop = value while others must be assigned through tag.setAttribute("prop", value);. The array above is a bit over-cautious, as I found that only onload and onerror needed to be set through dot notation (e.g. tag.onload = ...), the other attributes defined there can just as easily be set through setAttribute.

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

const safeAttrs = ['onload', 'onerror'];

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?

Hi @alexander-akait ,
hope you have nice summer weather =)
Could you please prioritise it a bit? Currently it's annoying as we have to use preload/prefetch with SRI, but chrome/safary/ff are not happy to preload with SRI but then load it without SRI.

@utlime need rebase, I think it is stale PR, so feel free to send this code again and we will merge it

@alexander-akait Not at all, I have been waiting for your response to my latest comment

I think we can write warning in docs about some attributes, they are still internal and should not change, it will simplify logic

alexander-akait · 2021-05-24T13:56:48Z

src/index.js

+            }
+
+            // Append dynamic attributes
+            MiniCssExtractPlugin.getCompilerHooks(compiler).customize.call(


Also let's rename customize to setLinkAttributes, it is more clear

patriksletmo · 2021-05-28T13:30:52Z

src/index.js

+            const attributes = {
+              href: `${RuntimeGlobals.publicPath} + ${RuntimeGlobals.require}.miniCssF(chunkId)`,
+              rel: JSON.stringify('stylesheet'),
+              onload: 'onLinkComplete',
+              onerror: 'onLinkComplete',
+            };


I think we can write warning in docs about some attributes, they are still internal and should not change, it will simplify logic

@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

// Link properties cannot be set using setAttribute, but must instead be set through // link.property = value. We maintain this list of known properties, so that we can // combine attributes of both types in the above object. const linkProperties = ['href', 'rel', 'type', 'onload', 'onerror'];

What do you think?

I think docs about these attributes will be enough, they are internal and rewriting them is not safe

Okay, so keep the above behavior but add section to docs?

@alexander-akait Hi there, any plans on pulling this in soon? I followed the discussion and it seemed like you were almost done.

解决了吗？目前也是这个sri问题，需要这个功能

alexander-akait · 2023-03-27T16:54:36Z

Sorry for delay, really, if somebody still need it feel free to send a new PR

/cc @patriksletmo maybe you want to finish it?

patriksletmo mentioned this pull request Apr 24, 2020

Hook for subresource integrity #40

Closed

jscheid mentioned this pull request Jul 5, 2020

Webpack 5 and webpack-subresource-integrity compatibility webpack/webpack#10826

Closed

patriksletmo force-pushed the master branch from 608bc70 to f893542 Compare April 12, 2021 12:44

feat: add hook for customizing injected runtime tags

a0ab666

Enable other plugins to tap into the tag injection mechanism in order to customize the functionality of lazy loaded chunks. Closes webpack-contrib#40.

patriksletmo force-pushed the master branch from f893542 to a0ab666 Compare April 14, 2021 09:46

alexander-akait reviewed Apr 14, 2021

View reviewed changes

alexander-akait reviewed May 24, 2021

View reviewed changes

patriksletmo commented May 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add hook for customizing injected runtime tags #526

feat: add hook for customizing injected runtime tags #526

patriksletmo commented Apr 24, 2020

jscheid commented Apr 24, 2020

patriksletmo commented Apr 26, 2020

jscheid commented Apr 26, 2020

patriksletmo commented Apr 26, 2020

jscheid commented Apr 26, 2020

jscheid commented Apr 26, 2020

patriksletmo commented Apr 27, 2020

jscheid commented Apr 28, 2020

jscheid commented Apr 28, 2020

utlime commented Mar 24, 2021

patriksletmo commented Mar 30, 2021

alexander-akait commented Mar 31, 2021

patriksletmo commented Mar 31, 2021

alexander-akait commented Mar 31, 2021

patriksletmo commented Apr 12, 2021

alexander-akait commented Apr 12, 2021

patriksletmo commented Apr 14, 2021

alexander-akait Apr 14, 2021

patriksletmo Apr 14, 2021

utlime May 24, 2021

alexander-akait May 24, 2021

patriksletmo May 24, 2021

alexander-akait May 24, 2021

alexander-akait May 24, 2021

patriksletmo May 28, 2021

alexander-akait May 28, 2021

patriksletmo May 28, 2021

alexander-akait May 29, 2021

pixagraphic Mar 28, 2022

Rainnut Mar 23, 2023

alexander-akait commented Mar 27, 2023

feat: add hook for customizing injected runtime tags #526

Are you sure you want to change the base?

feat: add hook for customizing injected runtime tags #526

Conversation

patriksletmo commented Apr 24, 2020

Motivation / Use-Case

Breaking Changes

Additional Info

jscheid commented Apr 24, 2020

patriksletmo commented Apr 26, 2020

jscheid commented Apr 26, 2020

patriksletmo commented Apr 26, 2020

jscheid commented Apr 26, 2020

jscheid commented Apr 26, 2020

patriksletmo commented Apr 27, 2020

jscheid commented Apr 28, 2020

jscheid commented Apr 28, 2020

utlime commented Mar 24, 2021

patriksletmo commented Mar 30, 2021

alexander-akait commented Mar 31, 2021

patriksletmo commented Mar 31, 2021

alexander-akait commented Mar 31, 2021

patriksletmo commented Apr 12, 2021

alexander-akait commented Apr 12, 2021

patriksletmo commented Apr 14, 2021

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

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

alexander-akait commented Mar 27, 2023