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

Only apply Fabric Mixin services on platforms that need them #790

Merged
merged 1 commit into from
May 22, 2023

Conversation

Juuxel
Copy link
Member

@Juuxel Juuxel commented Apr 4, 2023

Mixin provides its own bundled implementations on some other platforms (LaunchWrapper, ModLauncher) that can be used instead. As far as I can tell, this was the behaviour with LaunchWrapper before Knot was introduced in #37.

Mixin provides its own bundled implementations on some other platforms
(LaunchWrapper, ModLauncher) that can be used instead.
public class MixinServiceKnotBootstrap implements IMixinServiceBootstrap {
public MixinServiceKnotBootstrap() {
if (!FabricLauncherBase.getLauncher().useFabricMixinServices()) {
// If an exception is thrown here, Mixin is designed to skip over this service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the false return in MixinServiceKnot.isValid the way to prevent mixin from loading the service? Throwing an exception is costly and isn't a good way to prevent loading of service, as you have no way to distinguish it from other exceptions like ones generated from adding to an immutable list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixin itself does this in both the ModLauncher and LaunchWrapper implementations, albeit in bootstrap instead of the constructor and throwing a more precise exception (I somehow missed both of these details, will need to fix).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants