rspack icon indicating copy to clipboard operation
rspack copied to clipboard

[Feature]: Support `compilation.optimizeChunks` hook

Open bradzacher opened this issue 1 year ago • 13 comments

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.

bradzacher avatar May 10 '24 04:05 bradzacher

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?

hardfist avatar May 10 '24 11:05 hardfist

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

bradzacher avatar May 17 '24 04:05 bradzacher

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

hardfist avatar May 17 '24 05:05 hardfist

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.

LingyuCoder avatar May 21 '24 06:05 LingyuCoder

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.

bradzacher avatar May 28 '24 03:05 bradzacher

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!

stale[bot] avatar Jul 27 '24 09:07 stale[bot]

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

bradzacher avatar Jul 27 '24 10:07 bradzacher

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?

AdrianBannister avatar Aug 05 '24 03:08 AdrianBannister

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.

LingyuCoder avatar Aug 30 '24 08:08 LingyuCoder

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.

bradzacher avatar Sep 25 '24 12:09 bradzacher

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

hardfist avatar Sep 25 '24 12:09 hardfist