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

Make the Autoloads to static functions to not poisoning the autoloads of users #5

Open
1 of 2 tasks
kanimaru opened this issue Mar 29, 2024 · 7 comments
Open
1 of 2 tasks
Milestone

Comments

@kanimaru
Copy link
Owner

kanimaru commented Mar 29, 2024

  • TwitchService
  • HttpClientManager
@kanimaru kanimaru changed the title Make the Autoloads to static functions to not poising the autoloads of users Make the Autoloads to static functions to not poisoning the autoloads of users Aug 15, 2024
@lublak
Copy link
Contributor

lublak commented Aug 16, 2024

@kanimaru _static_init() can not be disabled by the developer, autoload can be disabled. Currently i use my own modification and disable static init so i can call it myself. Alternative soluton could be a one time setup like this: (i think we can just init it in the setup?)

static var log: TwitchLogger;
static var auth: TwitchAuth;
static var icon_loader: TwitchIconLoader;
static var irc: TwitchIRC;
static var eventsub: TwitchEventsub;
static var eventsub_debug: TwitchEventsub;
static var commands: TwitchCommandHandler;
static var cheer_repository: TwitchCheerRepository;
static var api: TwitchRestAPI

## Call this to setup the complete Twitch integration whenever you need.
## It boots everything up this Lib supports.
static func setup() -> void:
	if initialized: return
       # Setup Twitch setting before it is needed
	TwitchSetting.setup();
	log = TwitchLogger.new(TwitchSetting.LOGGER_NAME_SERVICE);
	log.i("Setup")
	auth = await TwitchAuth.new();
	api = TwitchRestAPI.new(auth);
	icon_loader = TwitchIconLoader.new(api);
	eventsub = TwitchEventsub.new(api);
	eventsub_debug = TwitchEventsub.new(api, false);
	commands = TwitchCommandHandler.new();
	irc = TwitchIRC.new(auth);

	initialized = true;
	log.i("Start")
	await auth.ensure_authentication();
	await _init_chat();
	_init_eventsub();
	if TwitchSetting.use_test_server:
		eventsub_debug.connect_to_eventsub(TwitchSetting.eventsub_test_server_url);
	_init_cheermotes();
	log.i("Initialized and ready")

@lublak
Copy link
Contributor

lublak commented Aug 16, 2024

The problem is that the token is refreshed directly and a new token may be fetched. However, this should only take place in a later part of the programme, but since I work with an autload (to call certain contents of the programme at any time), static is executed directly and I have no control over it. But I think the whole thing goes deeper than that. As I have already seen that the settings also have a static init. Manual configuration is therefore rather difficult.

@kanimaru
Copy link
Owner Author

You are right with this one. That developped over time initially everything should start with TwitchService.setup but somehow it shifted more and more into the static_init stuff. The static_init should technically just create all classes that are needed and setup should do it's real initialisation.

I'm currently not 100% sure how to fix it. But I find it much more ugly to have autoloads from some libraries.... My only Idea would be a bit overengineered thing. Like a class that initializes everything and you and you can override it or make your own implementation and pass it into the twitch_service.

@lublak
Copy link
Contributor

lublak commented Aug 16, 2024

In general, the only problem is that a new token is drawn, although I have not yet called a setup. static_init not only makes the classes available, but it happens a bit more in the background. But I understand that autoload is often cluttered, especially if every extension would use it. Or things like this also happen in the background: setting.load_from_wellknown(‘https://id.twitch.tv/oauth2/.well-known/openid-configuration’)
Personally, I find it unattractive to obtain something, especially if you don't need it. I don't know what would make sense here. Why is the setup function not sufficient here? In addition, _static_init is also asynchronous. This could lead to other errors, especially if you don't know when it's finished.
With ‘setup’ you could use await manually here. await TwitchService.setup()

@kanimaru
Copy link
Owner Author

I was thinking again about that problem and why I did it like this in the firstplace.
You know that every scene in godot should be able to be a standalone. When I would split the TwitchSetup method away then this wouldn't be possible anymore. Cause every scene that depends on it needs to call TwitchService.setup in this case.

Any other Ideas how to solve that problem. I mean autoloads would be appropriate for it but yeah. xD

@oakheartsoftware
Copy link

I'm not sure I fully understand why this is setup as static.

The class is inherently not static, it's members need to be setup and in specific states. The functions are also not really static, they rely on internal members that again belong to a specific state. Pushing all of this into a static class seems to break some core SOLID object oriented design principles.

It's very difficult to extend the TwitchService without rewriting the source itself. For example, if I wanted to override some behavior in the TwitchIconLoader, I would have to rewrite the _static_init() method in twitch_service.gd to pass in my own class.

Also, static means one instance, but what if I want multiple Twitch connections? Maybe to connect to two different channels, for example. But with this approach it's impossible to have different setups or configurations.

Instead, a user could build their own autoload creating the TwitchService themselves if they really wanted it to be a global script.

@kanimaru
Copy link
Owner Author

kanimaru commented Oct 9, 2024

Also, static means one instance, but what if I want multiple Twitch connections?

Im working right now on it already in version 2 of the lib. It was a horrible idea to make everything static.... I'm sorry for this inconvienence :/ Next version will be much more Node based for everything so that you can decide when to initialize what etc.

@kanimaru kanimaru added this to the v2 milestone Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants