Skip to content
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

[Bug]: missing plugin compliation hooks #6500

Open
bradzacher opened this issue May 10, 2024 · 11 comments
Open

[Bug]: missing plugin compliation hooks #6500

bradzacher opened this issue May 10, 2024 · 11 comments
Labels
bug Something isn't working

Comments

@bradzacher
Copy link

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 bradzacher added bug Something isn't working pending triage The issue/PR is currently untouched. labels May 10, 2024
@hardfist
Copy link
Contributor

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 bradzacher changed the title [Bug]: [Bug]: missing plugin compliation hooks May 17, 2024
@bradzacher
Copy link
Author

@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
Copy link
Contributor

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

@LingyuCoder
Copy link
Collaborator

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
Copy link
Author

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.

@LingyuCoder LingyuCoder removed the pending triage The issue/PR is currently untouched. label May 28, 2024
Copy link

stale bot commented Jul 27, 2024

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 stale bot added the stale label Jul 27, 2024
@bradzacher
Copy link
Author

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

@stale stale bot removed the stale label Jul 27, 2024
@AdrianBannister
Copy link

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
Copy link
Collaborator

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
Copy link
Author

bradzacher commented Sep 25, 2024

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants