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

add EnableRegistry for eventloop, fix #64 #65

Closed
wants to merge 2 commits into from

Conversation

zyxkad
Copy link

@zyxkad zyxkad commented Sep 28, 2023

No description provided.

@zyxkad
Copy link
Author

zyxkad commented Oct 22, 2023

Hi @dop251 can you merge this pull request? Or is there any issue on this pr that I have to fix?

@dop251
Copy link
Owner

dop251 commented Oct 23, 2023

I'm not sure this option is necessary. You can redefine or completely remove the require function to get the desired result.

@zyxkad
Copy link
Author

zyxkad commented Oct 23, 2023

I'm not sure this option is necessary. You can redefine or completely remove the require function to get the desired result.

I believe it is, if remove something will mean disable them, why do we need EnableConsole lol

@dop251
Copy link
Owner

dop251 commented Nov 1, 2023

In hindsight we probably didn't, but since it's already there I'm not going to remove it. What I definitely don't want to end up with is having an EventLoop option for every single Runtime or Registry option. It's also becoming quite messy due to options' inter-dependencies, for example with your latest change, NewEventLoop(EnableRegistry(false), EnableConsole(true)) will panic.

What about adding something like NewVanillaEventLoop() which does not do anything to the Runtime other than adding the loop-specific functions (setTimeout, etc.)? This will allow manual configuration of the Runtime and will cover all corner cases.

@zyxkad
Copy link
Author

zyxkad commented Nov 1, 2023

What about adding something like NewVanillaEventLoop() which does not do anything to the Runtime other than adding the loop-specific functions (setTimeout, etc.)? This will allow manual configuration of the Runtime and will cover all corner cases.

This is a very good idea:)

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

Successfully merging this pull request may close these issues.

2 participants