feat: add hook for customizing injected runtime tags

[patriksletmo]

This PR contains a:

• [ ] bugfix
• [x] new feature
• [ ] code refactor
• [ ] test update
• [ ] typo fix
• [ ] metadata update

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]

@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):

1. 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.
2. 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.
3. 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]

@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:

1. 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.
2. 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.
3. 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]

@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]

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]

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]

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]

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:

1. The global variable name must be unique
2. The map must never have a hook overridden
3. 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]

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]

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

[utlime]

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

[patriksletmo]

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

[alexander-akait]

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

[patriksletmo]

@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]

@patriksletmo make sense, can you rebase?

[patriksletmo]

@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]

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

[patriksletmo]

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