[Feature]: Support circular-dependency-plugin

[ska-kialo]

What problem does this feature solve?

While the optimizeModules hook was added in #2758 to support circular-dependency-plugin. The Module interface does not include the dependencies property the plugin uses to iterate through the dependencies of the module. Additionally the Compilation interface does not include the moduleGraph property the plugin uses.

What does the proposed API of configuration look like?

compilation.hooks.optimizeModules.tap('PluginName', (modules) => {
    const seenModules = {};
    for (const module of modules) {
        seenModules[module.debugId] = true;

        for (const dependency of module.dependencies) {
            if (
                dependency.constructor &&
                dependency.constructor.name === 'CommonJsSelfReferenceDependency'
            ) {
                continue;
            }

            const depModule = compilation.moduleGraph.getModule(dependency);

            // Check things on depModule
        }
    }
});

The plugin uses the following properties:

• Module.dependencies: in Webpack this is an array of HarmonyImportSideEffectDependency
• Module.debugId: a number
• Compilation.moduleGraph.getModule(dependency: HarmonyImportSideEffectDependency): Module: a method on the module graph that resolves a dependency to its Module

[h-a-n-a]

We're not intended to expose complex data structures like ModuleGraph, Dependency, ChunkGrpah, etc. You may port this plugin to Rust instead.

[saifislamrepos]

this proved to be a very important plugin in webpack recently i faced a issue because of circular dependency which took lot of time will be good to have feature in rspack also

[hardfist]

this plugin can actually be implemented using stats.json，someone want to give it a try？

[ska-kialo]

I implemented a very basic version using the stats at https://github.com/kialo/rspack-circular-dependency-plugin.

For our use case it works, but note that the tests that came with the original plugin do not pass, as I haven't found the time to fix them yet. Therefore it also doesn't have an official release. I'm happy to take contributions.

[saifislamrepos]

thanks @ska-kialo worked for me but was returning ids instead of readable names/path extracted modulesById[currentModule].name and resonModule.name working as expected now

[ska-kialo]

@saifislamrepos ah yes, I assume you're running in production mode? I hadn't considered that when implementing it.

[saifislamrepos]

@ska-kialo yes as in production it takes moduleids as deterministic i guess

[hardfist]

this will cause performance issue for rspack and we suggest do this check in lint tools like eslint | biome

[stevenpetryk]

I disagree with this choice. The bundler is what actually generates runtime code, and therefore, is the safest and most accurate place to check for cycles—which can cause crashes.

Additionally, how will it harm performance? Checking for cycles is O(V+E), so will scale ~roughly linearly in practice with the number of modules. If one throws a quick check after module resolution and before prod bundling, will that really take that much time?

I would ask that y'all reconsider this :) I think it would be valuable and can be done performantly.

[hardfist]

I disagree with this choice. The bundler is what actually generates runtime code, and therefore, is the safest and most accurate place to check for cycles—which can cause crashes.

Additionally, how will it harm performance? Checking for cycles is O(V+E), so will scale ~roughly linearly in practice with the number of modules. If one throws a quick check after module resolution and before prod bundling, will that really take that much time?

I would ask that y'all reconsider this :) I think it would be valuable and can be done performantly.

if it's implemented in rust side it could be done performantly, but if it need to be done in js side it will cause huge module graph communication which will hurt performance

[stevenpetryk]

Ah, I see. I think the general question of "should we support cyclic dependency checking" is worth considering.

Would the rspack team be open to that being a feature of rspack, even though it is not one of webpack? Or would the recommendation be to build this as a rust-based plugin ourselves? (For more clarity: we at Discord really want cycle checking).

[hardfist]

we will reconsider what is the best way to do it

[stale]

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!

[stevenpetryk]

Yes.