Configuration.ts leaking listeners

[cyansprite]

We create a lot of these the code path is pretty long Configuration -> Oni -> Plugin -> ... Basically, would it be more beneficial to switch this to a singleton? I'm doing this with the Config ( currently working on keybindings and abstracting it ) So I wonder if we could do the same for this?

[bryphe]

Oh ya, thanks for logging this issue!

I think one quick way to fix this would be, in the browser/src/Plugins/Api/Configuration.ts:

    public get onConfigurationChangedEvent(): IEvent<void> {
        return this._onConfigurationChangedEvent
    }

    private _onConfigurationChangedEvent: Event<void> = new Event<void>("oni_configuration_changed")

    constructor() {
        Config.instance().registerListener(() => {
            this._onConfigurationChangedEvent.dispatch(null)
        })
    }

    public getValue<T>(configValue: string, defaultValue?: T): T {
        return Config.instance().getValue(<any>configValue) || defaultValue
    }

This object gets created for every plugin, and it always starts listening to configuration changes... even if there is no listener registered.

I think it might be good instead to only start listening the first time we get the _onConfigurationChangedEvent (lazy init).

I don't think this particular object makes sense to be a singleton, because we basically pass it to the plugin via the Oni instance in the activate method.

One thing I'd like to move away from though, is using the Node EventEmitter class - and use the typed Event/IEvent objects in the code. You get better completion with those, and it's easier to document as opposed to a bunch of strings.

[cyansprite]

One thing I'd like to move away from though, is using the Node EventEmitter class - and use the typed Event/IEvent objects in the code. You get better completion with those, and it's easier to document as opposed to a bunch of strings. :+1:

I don't think this particular object makes sense to be a singleton, because we basically pass it to the plugin via the Oni instance in the activate method.

So each plugin has their own configuration?

[bryphe]

At least right now, there is an Oni object created for each plugin (and then each of those Oni objects creates that Configuration object, which listens to Config.ts).

Some of it is over-complicated, because previously, all the plugins were hosted in completely isolated contexts. I moved away from that now and I'd like to move towards having a common Oni object that is shared between the plugins - this would allow a model where plugins could define shared or common functionality (kind of like the Emacs plugin model). Just haven't had a chance to clean it up completely!

The side effect though is today, a new Oni object is created for each plugin, and passed to the plugin on activation. You can sort of see the lifecycle if you open the dev tools, put a breakpoint in the constructor in browser/src/Plugin/Api/Configuration.ts, and reload Oni - if you look up the callstack you'll see the flow of how it is created.

But I think the short term fix is to do the lazy initialization - and longer term, we wouldn't create new instances of this object for each plugin, we' d just have a common one (which would also help solve this problem, since we'd only register the listener once).

[cyansprite]

At least right now, there is an Oni object created for each plugin (and then each of those Oni objects creates that Configuration object, which listens to Config.ts).

Right I looked through the code to see the lifecycle :P I just didn't quite understand why.

Some of it is over-complicated, because previously, all the plugins were hosted in completely isolated contexts. I moved away from that now and I'd like to move towards having a common Oni object that is shared between the plugins - this would allow a model where plugins could define shared or common functionality (kind of like the Emacs plugin model). Just haven't had a chance to clean it up completely!

I think this is the best and yeah I understand, only so much you can do.

[bryphe]

Looks like this was addressed as part of #692