web-infra-dev / rspack

The fast Rust-based web bundler with webpack-compatible API 🦀️
https://rspack.dev
MIT License
9.77k stars 561 forks source link

[Bug]: missing plugin compliation hooks #6500

Open bradzacher opened 6 months ago

bradzacher commented 6 months ago

System Info

rspack version 0.6.0

Details

I was testing migrating our (quite complex and very bespoke) webpack setup to rspack.

Sadly I got the following error from one of our plugins:

compiler.hooks.thisCompilation.tap(name, compilation => {
    compilation.hooks.optimizeChunks.tap(
//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// TypeError: Cannot read properties of undefined (reading 'tap')

It's a custom chunking plugin - so I figured it wasn't necessary for my quick test and I disabled it. After disabling it I got a different error in a different plugin:

compiler.hooks.thisCompilation.tap(name, compilation => {
    compilation.hooks.runtimeRequirementInTree.for(
//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// TypeError: Cannot read properties of undefined (reading 'for')

I didn't proceed any further after this as this second plugin is required. Is there something I need to do to get these hooks working? Or are they not supported yet?

Reproduce link

No response

Reproduce Steps

I can provide a more complete repro if it's required - it's a bit hard to pull apart the logic from within our internal plugins for a repro.

hardfist commented 6 months ago

runtimeRequirementInTree hook is complex and it's not supported yet, your plugin may could be implemented using other hooks,can you tell us what functionality do your plugin implement?

bradzacher commented 5 months ago

@hardfist sorry I lost track of this

I'm not entirely sure as all this infra predates my tenure at Canva - I'm just looking to make things faster!

It looks like the plugin (called RtlCssPlugin) uses rtlcss to create two versions of every .css file - a <file>.ltr.css and a <file>.rtl.css

The usage of that specific hook is:

compiler.hooks.thisCompliation.tap(RtlCssPlugin.name, compliation => {
  const enabledChukns = new WeakSet();
  const addRtlCssRuntime = (chunk: Chunk) => {
    if (enabledChunks.has(chunk)) {
      return;
    }
    enabledChunks.add(chunk);
    compilation.addRuntimeModule(chunk, new RtlCssLoadingRuntimeModule());
    compilation.addRuntimeModule(chunk, new GetChunkFilenameRuntimeModule( ... ));
  };
  compilation.hooks.runtimeRequirementInTree
    .for(RuntimeGlobals.ensureChunkHandlers)
    .tap(RtlCssPlugin.name, addRtlCssRuntime);
  compilation.hooks.runtimeRequirementInTree
    .for(RuntimeGlobals.hmrDownloadUpdateHandlers)
    .tap(RtlCssPlugin.name, addRtlCssRuntime);
});

where RtlCssLoadingRuntimeModule essentially defines a small module which sets a global variable.

If there's a better way to do this I'm more than happy to refactor it - I just don't know enough about webpack plugins to know what the best course of action is!

hardfist commented 5 months ago

I'm not sure whether this can be implemented by runtimeModule hooks, @LingyuCoder any ideas?

LingyuCoder commented 5 months ago

I'm not sure whether this can be implemented by runtimeModule hooks, @LingyuCoder any ideas?

Runtime modules use the whole compilation object to generate their contents. So the compilation.addRuntimeModule() can not be port easily.

The compilation.runtimeModule hook can only modify the content of module which has been exists. Perhaps you can try concatenating the content of RtlCssLoadingRuntimeModule and GetChunkFilenameRuntimeModule to the end of the content of EnsureChunkRuntimeModule and CssLoadingRuntimeModule.

bradzacher commented 5 months ago

I'm on parental leave ATM (so I'm not working on company stuff) - I'll get back to you in a few weeks once I've returned and chatted to the subject matter experts.

stale[bot] commented 3 months ago

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

bradzacher commented 3 months ago

I'll cycle back with the team to get the discussion going again.

AdrianBannister commented 3 months ago

Hey @LingyuCoder, thanks for the suggestion. It's a bit ugly to write our own runtimes onto the back of the Webpack runtime, but we'll work with that. One concern I have is that according to the rspack documentation the runtimeModule hook is read only. Will this behaviour of modifying the existing runtime always work or will there be a different approach to take in the future?

Am I also right to assume that helper classes like runtime.GetChunkFilenameRuntimeModule will also not be available in rspack?

LingyuCoder commented 2 months ago

Hey @LingyuCoder, thanks for the suggestion. It's a bit ugly to write our own runtimes onto the back of the Webpack runtime, but we'll work with that. One concern I have is that according to the rspack documentation the runtimeModule hook is read only. Will this behaviour of modifying the existing runtime always work or will there be a different approach to take in the future?

Am I also right to assume that helper classes like runtime.GetChunkFilenameRuntimeModule will also not be available in rspack?

If you just want to override the GetChunkFilenameRuntimeModule, you can try to assign to __webpack_get_script_filename__. This is the recommended way in rspack and webpack. The runtimeModule hook will lead to performance regression. The runtime module is stable and will not changed during 1.x.x.

bradzacher commented 1 month ago

With the release of 1.0.7 both addRuntimeModule and runtimeRequirementInTree are now included in rspack, correct?

So then I believe all we need is optimizeChunks and we can start testing further.

hardfist commented 1 month ago

the current implementation still has lots of limitation yet(which we're working to solve), if you met any problems let us know