-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Split Bundle
and StaticBundle
#19761
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The parallel contents of code for StaticBundle
vs Bundle
is kinda annoying, but I know that's changing in the next prs, so I'm not worried about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small notes on docs that I think we should clean up quick. I would really appreciate a paragraph or two of module docs laying out these traits and how they interconnect, but that's fine to leave to follow-up if you prefer.
More importantly, I'm not sure why we should keep a split between DynamicBundle
and Bundle
around after these changes. It looks like Bundle
should now be object-safe, allowing us to merge them together.
I haven't done a deep dive on this area of the code in a while though, so please feel free to explain why we can't / shouldn't do that :)
However even then it will be a bit of a hassle to remove |
Objective
Option
bundles andBox<dyn Bundle>
#19491Bundle
into one type representing a static set of component types (StaticBundle
) and one that will be used for representing a possibly dynamic set of components.Solution
StaticBundle
type, mirroringBundle
for nowBundle
(i.e. from its type) to useStaticBundle
&self
parameters toBundle
to ensure all APIs that use it have a concrete instance to work with in the future.Additional notes
For now this doesn't do anything functionally, as
Bundle
still requires the components to be statically fixed. However this opens the doors for changes to theBundle
trait that allow dynamic components without impacting APIs that actually rely on static components.