Skip to content

feat: implement CircularDependencyRspackPlugin#8876

Merged
LingyuCoder merged 9 commits intoweb-infra-dev:mainfrom
faultyserver:faulty/circular-dependencies-plugin
Mar 24, 2025
Merged

feat: implement CircularDependencyRspackPlugin#8876
LingyuCoder merged 9 commits intoweb-infra-dev:mainfrom
faultyserver:faulty/circular-dependencies-plugin

Conversation

@faultyserver
Copy link
Copy Markdown
Contributor

@faultyserver faultyserver commented Dec 30, 2024

Summary

This PR implements CircularDependencyRspackPlugin, an adaptation of circular-dependency-plugin for Webpack implemented natively in Rust to work with the Rspack module graph.

circular-dependency-plugin is a moderately-used plugin (~500k weekly downloads) to detect circular import dependencies in bundled code. It's not perfect, and the output isn't always meaningful (i.e., import cycles aren't forbidden in JS, so long as initialization can occur without creating a cycle. This plugin cannot detect that, as is), but identifying cycles is helpful for debugging and resolving bugs and errors caused by circular dependencies.

There's been an issue requesting support for this plugin for most of this year, but the major blocker for being able to run the existing plugin is exposing the module graph and dependencies on the JS side, which incurs a major serialization cost. One suggestion was to use the existing stats.json object to reverse-build the module graph, but that is also error-prone and not easy to parse, alongside still requiring expensive up-front serialization. So, this PR rewrites the plugin natively in Rust, specifically for Rspack's module graph.

At the same time, the existing plugin is known to be very slow, and only really feasible to run on production builds rather than in a hot-reloading development load. Instead, the implementation for detecting cycles here has been updated to be far more efficient, requiring only a single traversal of the module graph rather than one traversal per module. With that change, and the memory efficiency of reusing the existing module graph, performance is imperceptible, even for projects with thousands of modules and hundreds of thousands of connections between them (in testing, our codebase has ~20k modules and ~230k connections, and the plugin executes with the JS callback hooks in under 50ms). This implementation means it can easily be used in development mode and hot-reloading environments and provide better feedback in the moment.

Divergence:

This implementation is effectively the same as the webpack version, but it has some differences:

  • It works with module identifiers rather than relative paths (meaning the cwd option also isn't implemented). There isn't a big reason for this other than it being expensive to copy Strings all over the place for comparing with the exclude/connection patterns, and the borrow checker really doesn't like keeping Cow references around when the JS callbacks are invoked with a mutable compilation.
  • The include option is not present, because the original plugin only used this to filter the list of modules it traversed. It can be added without much issue, but doesn't really do much meaningfully.
  • Added ignoredConnections to give more granular control over ignoring specific cycles, since ignoring an entire module is often too heavy-handed and would prevent detecting any cycles through that module, which seems antithetical to the intent of the plugin (you can never know absolutely that a module isn't problematically cyclical, you can only guess, at which point the plugin gives you nothing).
  • The plugin detects all cycles and then filters them out, allowing for an additional onIgnored callback to handle them if desired.
  • The onDetected callback also includes an entrypoint parameter first indicating which entrypoint the cycle occurred in (the same cycle will be reported for each entrypoint it occurs in).

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 30, 2024

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link
Copy Markdown

netlify bot commented Dec 30, 2024

Deploy Preview for rspack ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 93b5ff4
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/67e0edefd71cfc00088af41a
😎 Deploy Preview https://deploy-preview-8876--rspack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@faultyserver faultyserver changed the title implement CircularDependencyRspackPlugin feat: implement CircularDependencyRspackPlugin Dec 30, 2024
@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Dec 30, 2024
@hardfist hardfist requested a review from ahabhgk December 30, 2024 05:46
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Dec 30, 2024

CodSpeed Performance Report

Merging #8876 will not alter performance

Comparing faultyserver:faulty/circular-dependencies-plugin (93b5ff4) with main (ae0330b)

Summary

✅ 8 untouched benchmarks

@JSerFeng
Copy link
Copy Markdown
Contributor

JSerFeng commented Jan 5, 2025

LGTM, I can approve once CI succeed

@honzahruby
Copy link
Copy Markdown

Hey @faultyserver, any progress on this one? 🙏

@illogic-al
Copy link
Copy Markdown
Contributor

illogic-al commented Mar 5, 2025

@JSerFeng @faultyserver I've made an update at faultyserver#1 to bypass the failing CI checks in this commit. Any chance to get it applied so this PR can move forward?

I can also try to resolve any conflicts present if the main branch gets too far ahead.

@faultyserver
Copy link
Copy Markdown
Contributor Author

faultyserver commented Mar 5, 2025

I think the goal is to add integration tests before merging, matching the ones that exist in the webpack version of the plugin: https://github.com/aackerman/circular-dependency-plugin/tree/master/__tests__.

I don't currently have time to get those implemented and passing, but happy to look if someone else wants to pick that part up. Alternatively it could be simpler to merge and add tests later rather than trying to work through my forked branch.

@JSerFeng
Copy link
Copy Markdown
Contributor

I'll complete this next week, If somebody want to contribute please let me know

@illogic-al
Copy link
Copy Markdown
Contributor

I'll complete this next week, If somebody want to contribute please let me know

I mean, I don't mind taking a stab at writing these tests. But the most work I've done with rspack is fixing a few documentation typos so, y'know, I would probably need some help.

@hardfist hardfist requested a review from fireairforce March 19, 2025 03:08
@hardfist hardfist assigned fireairforce and unassigned JSerFeng Mar 19, 2025
@hardfist
Copy link
Copy Markdown
Contributor

I think the goal is to add integration tests before merging, matching the ones that exist in the webpack version of the plugin: aackerman/circular-dependency-plugin@master/__tests__.

I don't currently have time to get those implemented and passing, but happy to look if someone else wants to pick that part up. Alternatively it could be simpler to merge and add tests later rather than trying to work through my forked branch.

@faultyserver we can continue to add tests case to your pr, if you don't mind

@faultyserver
Copy link
Copy Markdown
Contributor Author

please do, yes! i unfortunately still don't have much time to continue development here myself, but if others are willing and able to make the needed changes that sounds great to me

@fireairforce fireairforce force-pushed the faulty/circular-dependencies-plugin branch 2 times, most recently from f02d0d8 to dd63d89 Compare March 19, 2025 08:48
@ahabhgk ahabhgk removed their request for review March 19, 2025 10:58
@fireairforce fireairforce force-pushed the faulty/circular-dependencies-plugin branch from 549bdce to bbca0c1 Compare March 19, 2025 16:17
@fireairforce
Copy link
Copy Markdown
Contributor

fireairforce commented Mar 19, 2025

Implement of @faultyserver in the pr description is very detailed, i just do something to follow the job:

  • complete the CN doc
  • add test case for the plugin, test case can be found at packages/rspack-test-tools/tests/diagnosticsCases/plugins/circular-dependency-plugin , i add some warning case to get the circular_dependency_plugin diagnostic output.

@fireairforce fireairforce force-pushed the faulty/circular-dependencies-plugin branch from 43397ff to f4abb45 Compare March 20, 2025 03:40
@fireairforce fireairforce force-pushed the faulty/circular-dependencies-plugin branch from f4abb45 to 93b5ff4 Compare March 24, 2025 05:30
@LingyuCoder LingyuCoder merged commit 0539c2f into web-infra-dev:main Mar 24, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: feature release: feature related release(mr only)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants