DEWP: Handle cyclical module dependencies #65291
Merged
+112
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
michalczaplinski
changed the title
michalczaplinski
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.77 MB ℹ️ View Unchanged
|
sirreal
reviewed
Sep 13, 2024
| const DependencyExtractionWebpackPlugin = require( '../../..' ); | ||
|
|
||
| module.exports = { | ||
| plugins: [ new DependencyExtractionWebpackPlugin() ], |
There was a problem hiding this comment.
I think there will need to be an external module listed here that's used in one or both of the file and we can probably disable defaults as well.
There was a problem hiding this comment.
Thanks Jon! Yeah, I've added it now!
I think I didn't realize this before: The problem that I found only affects the externalized modules. Do you think that's correct?
|
I was able to reproduce the error (originally found in here) in a new test case |
Enhanced the dependency resolution logic to detect cycles in the module graph, preventing infinite loops during static dependency path checks. Introduced a Set to track visited blocks and avoid revisiting them.
| * @param {webpack.DependenciesBlock} block | ||
| * @param {webpack.Compilation} compilation | ||
| * @param {webpack.DependenciesBlock} block | ||
| * @param {Set<webpack.DependenciesBlock>} visited |
There was a problem hiding this comment.
It might be better to only store IDs of each visited block instead of the whole block. This would use less memory.
However, I could not see if the DependenciesBlock has a stable ID of any kind: https://github.com/webpack/webpack/blob/899f06934391baede59da3dcd35b5ef51c675dbe/lib/DependenciesBlock.js#L29
Without benchmarking, it's also hard to know if it makes a difference.
There was a problem hiding this comment.
I have some ideas for this, I'm working on it now.
sirreal
reviewed
Sep 16, 2024
Comment on lines
+352
to
+353
| static #staticDepsCurrent = new WeakSet(); | ||
| static #staticDepsCache = new WeakMap(); |
There was a problem hiding this comment.
@michalczaplinski I used the same basic idea but with a few tweaks:
- WeakSet for what's currently being explored. If the block is being explored and is being searched again, it does not represent a static path to the root. This should allow traversal to continue along other paths even if ultimately the current block is a static path to the root.
- WeakMap for what's already been explored. This shouldn't be strictly necessary, but serves as a cache to avoid traversing the same paths over and over.
These weak data structures should work very well here, as long as the webpack compilation holds a reference to the blocks they can be cached, but will be garbage collected when webpack no longer holds a ref.
The current set could likely be a regular set since we should always clean up, but I don't think there's harm in it being weak.
There was a problem hiding this comment.
Nice! Yeah, the weak structures work well here 👍
The cache is a nice touch. We have a space-time complexity tradeoff, and I agree that using more memory in favor of time makes sense.
sirreal
approved these changes
Sep 18, 2024
|
Yep, looks right 👍 |
noisysocks
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 23, 2024
* Add tests for cyclical dependencies * Add the externals and make the test fail * Add cycle detection in dependency path checking Enhanced the dependency resolution logic to detect cycles in the module graph, preventing infinite loops during static dependency path checks. Introduced a Set to track visited blocks and avoid revisiting them. * Revert changes * Propose static WeakSet/WeakMap implementation. * Add CHANGELOG entry * Remove redundant plugin config in test * Revert "Remove redundant plugin config in test" This reverts commit b5e33db. * Remove redundant plugin config in test * Updated the snapshot files --------- Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: e4df8fd |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #65288
How?
By tracking which dependencies have already been visited.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast