Sort the connections back into topological order after traversing runtimes #75
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
mattcompiles
approved these changes
Sep 16, 2024
yamadapc
reviewed
Sep 16, 2024
| // As manifest bundles may be added during runtimes we process them in reverse topological | ||
| // sort order. This allows bundles to be added to their bundle groups before they are referenced | ||
| // by other bundle groups by loader runtimes | ||
| // Sort bundles into reverse topological order |
There was a problem hiding this comment.
This is topological order, not reverse
| // Sort the bundles back into forward topological order | ||
| let connections: Array<RuntimeConnection> = []; | ||
|
|
||
| bundleGraph.traverseBundles({ |
There was a problem hiding this comment.
This isn't reverse topological order, I don't think
There was a problem hiding this comment.
This should be
const connections = [];
bundleGraph.traverseBundles({
exit(bundle) {
connections.push(...connectionMap.get(bundle));
}
});
connections.reverse();
| @@ -192,8 +221,14 @@ export default async function applyRuntimes<TResult: RequestResult>({ | |||
| } | |||
| } | |||
|
|
|||
| // Correct connection order after generating runtimes in reverse order | |||
| connections.reverse(); | |||
| // Sort the bundles back into forward topological order | |||
There was a problem hiding this comment.
You're now going for reverse topological order
Nikola-3
pushed a commit
that referenced
this pull request
Oct 29, 2024
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.
A while back in Parcel, we reversed the order that runtimes were processed in, so that child runtimes could be processed before parent bundles who would need to know about them. That didn't quite cut it, so we moved to reverse topological order.
This works fine for Jira, but in Confluence it causes an issue where a bundle gets loaded thinking that some of its dependencies are already loaded when they aren't.
We thought we'd fixed this with
connections.reverse(), but that doesn't actually restore topological order, and it seems that Confluence has found the edge case.The fix here is to actually restore topological order, by processing the bundles in reverse, and then re-traversing and building up the connection list that way.