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

feat: implement CircularDependencyRspackPlugin #8876

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

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

CLAassistant commented Dec 30, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented Dec 30, 2024

Deploy Preview for rspack ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d308c8e
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/67c8be66e35b8800080d8416
😎 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
Copy link

codspeed-hq bot commented Dec 30, 2024

CodSpeed Performance Report

Merging #8876 will not alter performance

Comparing faultyserver:faulty/circular-dependencies-plugin (821af75) with main (437c370)

🎉 Hooray! codspeed-node just leveled up to 4.0.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 3 untouched benchmarks

@JSerFeng
Copy link
Contributor

JSerFeng commented Jan 5, 2025

LGTM, I can approve once CI succeed

@honzahruby
Copy link

Hey @faultyserver, any progress on this one? 🙏

@illogic-al
Copy link

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.

…ugin

chore: Rename file and fix some typos to continue upstream CI checks
@faultyserver
Copy link
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.

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.

5 participants