-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Store hooks as property of compilation #1869
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.
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
base: main
Are you sure you want to change the base?
Conversation
This allows reading the hooks more reliably. Using a WeakMap in a closure causes issues if multiple versions of html-webpack-plugin are installed because it requires that plugins that tap into these hooks use the version of this package that is resolved by the webpack config. This may be a common issue in monorepos. Using a global symbol allows any version of `html-webpack-plugin` to resolve the hooks used in the compilation, allows plugins in monorepos to work more seamlessly.
I don't think that the html-webpack-plugin should alter the webpack compilation object What about passing the html-webpack-plugin to the custom plugin? class MyHTMLPlugin {
constructor(plugin) {
this.plugin = plugin
}
} |
This comment was marked as outdated.
This comment was marked as outdated.
@jantimon Any updates on this? |
@jantimon I was affected by this issue again so thought I'd revisit it. I'll reiterate an old proposal and list another alternative, which can serve as a point of reference for others but to consider tradeoffs of not addressing this. A solution I mentioned before is to store the const hooksSymbol = Symbol.for('html-webpack-plugin/hooks');
globalThis[hooksSymbol] ??= new WeakMap();
const hooksMap = globalThis[hooksSymbol]; Another workaround I've applied in my plugins is to accept a const plugin = new MyCustomPlugin({
getHtmlWebpackPluginHooks: (compilation) => {
return LocalWebpackPlugin.getHooks(compilation)
}
}); This works fine but feels like an unnecessary burden to place on plugin developers. Let me know if you have any thoughts. If you oppose any change to this please just close the PR 🙏🏽 |
Can you run |
@alexander-akait I dont have access to this environment at the moment. But the issue is relatively straightforward. Using a traditional Something like
Hope this helps if you are seeking to understand the problem |
@GeorgeTaveras1231 it means this should be resolved by using only one version, unfortunately there are some limitation with old plugins, especial v1, why you can't use the only one version? |
@alexander-akait in theory I agree with you. You could force the usage of a single version by marking it as a However, in practice, any package listed in You could place it in the root workspace as a You could also create a whole new workspace just for testing - but I imagine for some projects will co-locate tests inside the workspace being tested - so this approach could be undesired in those cases. I think this ultimately leaves us with limited options for solving this
After working in a monorepo for a couple years, I feel pretty confident in my assessment of this problem but let me know if you see any gaps in my thought process. Ultimately, I'm not blocked by this because I worked around the issue with the 'dependency injection' approach, and am just interested in giving back. So I'll accept whatever you feel is appropriate 😌 |
I accidentally left out of the solutions available in the user space the original work around I added to the PR description which is to resolve |
I can understand the pain and get that this really happens in large codebases. However I believe there are better approaches than using global state and would prefer to keep the current isolation for stability. Rather than working around multiple versions I would enforcing a single version.. That's possible for all mono repository managers. With pnpm you can use: {
"pnpm": {
"overrides": {
"html-webpack-plugin": "^5.5.0"
}
}
} Yarn has The main issue with your proposed global approach is that some plugins legitimately use html-webpack-plugin internally and apply their own instances: const HtmlWebpackPlugin = require("html-webpack-plugin");
class CustomTemplatePlugin {
apply(compiler) {
// This plugin creates its own HtmlWebpackPlugin instance
const htmlPlugin = new HtmlWebpackPlugin({
template: 'custom-template.html',
filename: 'special-output.html'
});
// Applies it to the compiler
compiler.apply(htmlPlugin);
// Later wants to hook into its own instance
compiler.hooks.compilation.tap('CustomTemplatePlugin', (compilation) => {
const hooks = HtmlWebpackPlugin.getCompilationHooks(compilation);
hooks.beforeEmit.tap('CustomTemplatePlugin', (data) => {
// Expects to work with its own instance's hooks
});
});
}
} With your proposed global approach, this plugin might overwriting foreign hooks from a different html-webpack-plugin in the global state, causing runtime errors, incompatible hook signatures and data format mismatches between versions or other unexpected behavior when multiple html-webpack-plugin instances exist.. The current WeakMap approach ensures that each plugin gets exactly the hooks from the version it's designed to work with If package manager overrides are not possible for you I believe using the html-webpack-plugin from root like you showed is also fine: const require = createRequire(path.resolve(compiler.options.context, 'webpack.config.js');
const HtmlWebpackPlugin = require('html-webpack-plugin'); I know that's not what you were looking for but I hope it helps a little bit to understand the intention of non global hooks |
@jantimon your comment totally makes sense and I understand the risk with globally storing the hooks. I'll just say I think I spotted a contradiction in your recommendation. If the recommendation is to use a single version/instance of I do see the risk of storing it globally following my recommendation leading to weird issues especially if the hook API changes significantly between major versions and a user loads more than one version in a single runtime. I think this can be managed by changing the global symbol every time there is a significant change to the hook API (required feature or breaking change) - yes this adds some maintenance overhead since you have to to keep some loose versioning scheme outside of the package.json but would allow multiple instances to communicate while mitigating some of the risk. As far as the recommendation to use resolutions and overrides. You are right to suggest that will fix the issue but these workarounds should be reserved for special cases. And are easier to manage in smaller repos. In general, this is a pattern that limits the growth of monorepos because it requires all sub projects to be in sync. This limits the ability to do partial migration between major releases (for example, migrate a workspace at a time, merge/integrate, then iterate). |
I think I may have misinterpreted a bit of your message and am now not sure if you meant to suggest that using a single instance is a general rule. I think this is ok - but I suppose I'm still hoping something can be done. At least if possible, add guidelines for plugin developers that align with proper plugin implementations that do not lead to this issue. I think this applies to all plugin developers (wether they use In order to ensure that different plugins work well with each other and to avoid this error, they should either.
Without this, there is the risk that a minor version mismatch or accidentally incompatible version range request leads to two or more plugins unable to properly tap into the html hooks and build. I have a vague memory of running into this issue with some open source plugins that were unable to be used together for this reason - though I don't have the example readily available 😅. Would these guidelines seem reasonable to you? And does changing this request to a documentation request resonate with you? |
Your global approach has a serious flaw: If multiple versions of So what might happen is that version A creates hooks, version B loads later and overwrites them, but version A is used in the webpack config - therefore hooks called by A might have the wrong API or might not exist in that version and break the build From my feeling the real issue is that you're requiring the wrong module. You probably have something like:
Your plugin gets the local module, but webpack is using a different instance from the config. This is a monorepo module resolution problem and less an issue of the And if it is a monorepo issue I would try to fix the monorepo. First you could use If you really want to fix it in code you can also try this approach that finds the actual plugin instance webpack is using but also loses the version guarantee: function findHtmlWebpackPlugin(compiler) {
for (const plugin of compiler.options.plugins || []) {
if (plugin.constructor.name === 'HtmlWebpackPlugin' ||
plugin.__proto__.constructor.name === 'HtmlWebpackPlugin') {
return plugin.constructor;
}
}
return null;
}
class MyHTMLPlugin {
apply(compiler) {
compiler.hooks.compilation.tap('MyHTMLPlugin', (compilation) => {
const HtmlWebpackPlugin = findHtmlWebpackPlugin(compiler);
const hooks = HtmlWebpackPlugin?.getCompilationHooks(compilation);
hooks?.beforeEmit.tap('MyHTMLPlugin', () => { /* Omitted */ });
});
}
} Note that this will find only the first instance - so if you are using multiple html-webpack-plugin versions you might want to adjust |
@jantimon I hope you get a chance to review my other comment where I propose documenting guidelines. Your comment is completely correct and am aware of the potential risks of the wrong version loading first - I still think it can be mitigated/managed with minimal to no side-effects but I wont insist on this. But I will insist that - this isn't strictly a monorepo problem. This can happen in any scenario where developers install 2 or more plugins, if those plugins intend to tap into the hooks of the root webpack compilation using the This is why I recommended documenting guidelines for plugin/library developers. |
This allows reading the hooks more reliably.
Using a WeakMap in a closure causes issues if multiple versions of html-webpack-plugin are installed because it requires that plugins that tap into these hooks use the version of this package that is resolved by the webpack config. This may be a common issue in monorepos. Using a global symbol allows any version of
html-webpack-plugin
to resolve the hooks used in the compilation, allows plugins in monorepos to work more seamlessly.Current workaround
I work in a monorepo where I've implemented a couple of plugins that tap in the
html-webpack-plugin
hooks. To bypass the issues I'm encountering, I defer the loading ofhtml-webpack-plugin
to the execution of the plugin, and use the context of the compilation to require to appropriate instance ofhtml-webpack-plugin