search

dop251 / goja_nodejs

Nodejs compatibility library for Goja
MIT License
329 stars 78 forks source link

add `EnableRegistry` for eventloop, fix #64 #65

Closed zyxkad closed 10 months ago

zyxkad commented 11 months ago

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

dop251 commented 10 months ago

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

zyxkad commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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:)