Loading and unloading plugins/modules dynamically?

[109C]

[demipixel]

For core plugins, I'm not sure if this well ever change. For plugins (once we implement them) we might do this.

The biggest issue is that we would manually have to keep track of every event place on the server/players and remove them if a plugin is "disabled"... which I'm not sure if that's possible.

[rom1504]

Related to https://github.com/mhsjlw/flying-squid/issues/29

That's one of the reasons I think "not enabling" is better than "cancelling".

We may want to have nice bricks of enableable behaviours like diggingBlock, placingBlock, moving, changingWorld, ... that will all be not enabled by default and then we can have a default list of behavior to have a normal vanilla server. If people want to disable something, even dynamically like in this issue, it won't be a problem.

Now I guess it's a bit parallel to cancelling because we might want to only disable things in some particular cases dynamically. (which mean we might need both)

But I think it might make the code cleaner and let us try thing easily (for example, if you want to test a particular feature, it would be easy to disable everything but that feature)

[demipixel]

The only real reason we need this is if we want to "reload" plugins (it would basically allow us to test out plugins' code without restarting the server, making development a ton faster). However, in order to do this, we'd need some way to track events and remove them.

For example, if it's a player plugin, inbetween plugins being called, we change "player.on" or "player.when" or whatever it might be so that it saves the listener somewhere. So if I have playerA and playerB and two plugins, it might look like:

playerA.events == {
  pluginA: [listen1],
  pluginB: [listen1, listen2]
}

playerB.events == {
  pluginA: [listen1],
  pluginB: [listen1, listen2]
}

As soon as we want to disable or, more likely, restart a plugin (e.g. pluginA), we simply plugin.disableFunction() it and then remove all of the listeners in our easily accessible object (e.g. go through each player and remove every listener of pluginA).

A little over the top, perhaps :P

[rom1504]

Yeah, I think something like that would work. That's similar to what I was proposing there https://github.com/mhsjlw/flying-squid/issues/32#issuecomment-151116980

[rom1504]

The thing here is plugins don't just add listeners. They add functions to the serv and player object, they add commands, ... Maybe plugins shouldn't add stuff directly to these objects, and instead they should add stuff through a light interface that would store what the plugins added and be able to remove these stuff.

For example it might go like:

this.addFunction(player,"sendBlock", (position, blockType, blockData) => {...});
this.addCommand(player,{base: 'setblock',info: 'to put a block',...});
this.addListener(player._client,'arm_animation',() => { ... });

These 3 functions would both add the functions/listeners/commands in the real objects (player,player.commands and player._client in this example) and remember the name of these additions coming from this plugin in an object like:

{
  functions:[{"obj":player,"functionName":"sendBlock"}],
  commands:[{"obj":player,"commandName":'setblock'}],
  listeners:[{"obj":player._client,"eventName":'armAnimation',"listener":f}]
}

by plugin

Then we could easily (automatically) remove these added stuff when we need to remove the plugin.

EDIT: well I'm not 100% sure about that idea, it still seems too complex and unmodular to me.

[demipixel]

Yeah... As much as I want reloading plugins, I don't know if I'd go that far. As it stands, though, setting player.sendBlock twice wouldn't hurt.

The other two things left are listeners and local data. Once we have saving, we can just tell it to save the data and we'll load it when we boot up the plugin again....

... which still leaves us with listeners being the issue.

[rom1504]

"setting player.sendBlock twice wouldn't hurt" that means we wouldn't be able to disable(/unload) plugins, only reload them.

anyway, if we only cared about listeners, we could still use my method but maybe with nicer names :

this.on(player._client,'arm_animation',() => { ... });

There's not really any way around writing the this (or the plugin name but I don't think that's better), if we want to track what plugin added which listener to which object.

[rom1504]

Ohh I have an idea. It's a bit hacky and it assumes plugins do listen on loading and not after but what would be possible is comparing the list of listeners before and after loading a plugin. That way you'd know what the plugin added, and you could remove it when needed.

(and actually the same idea would apply for functions and whatever else)

Ah even simpler: we just have to listen on https://nodejs.org/api/events.html#events_event_newlistener during a plugin injection.

[rom1504]

See a proof of concept of that idea there https://github.com/mhsjlw/flying-squid/pull/133 Results (for player._client) :

animations : arm_animation
animations : entity_action
chat : chat
chest : block_place
digging : block_dig
inventory : held_item_slot
inventory : window_click
inventory : set_creative_slot
logout : end
placeBlock : block_place
pvp : use_entity
respawn : client_command
settings : settings
stats : client_command
updatePositions : look
updatePositions : position
updatePositions : position_look

[demipixel]

@rom1504 But if we're doing it that way, we could've just done the old method:

for (var p in plugins) {
  serv.on = (name, func) => {
    plugins[p].listeners.push({ name: name, func: func });
    serv.when.apply(this, name, func);
  }
  plugin[p].server(serv, settings);
}

But I sort of threw the idea away because I knew that at some point, people will want to make listeners whenever they want, not just on injects...

[rom1504]

yeah that would work too. Do you think we will need to have listeners not on injects often ?

[demipixel]

Yes

[rom1504]

Ah OK so I guess we do have to do something like https://github.com/mhsjlw/flying-squid/issues/83#issuecomment-159889100 then

[demipixel]

Can we wait a bit before we do this? I wanna see if I can think of any better way...

[rom1504]

Yeah yeah sure, I wasn't planning on doing it right now anyway ^^ just writing ideas

[deathcap]

It's a bit hacky and it assumes plugins do listen on loading and not after but what would be possible is comparing the list of listeners before and after loading a plugin.

Maybe I'm missing something but can't you just have the plugin remove the event listeners it has added? This is what I am doing in voxel.js plugins (ref https://github.com/PrismarineJS/flying-squid/issues/29#issuecomment-176891973), for example in https://github.com/voxel/voxel-chunkborder/blob/master/chunkborder.js#L32-L46:

BorderPlugin.prototype.enable = function() {
  this.shell.bind('chunkborder', 'F9');
  if (this.keysPlugin) this.keysPlugin.down.on('chunkborder', this.onToggle = this.toggle.bind(this));
  this.shell.on('gl-init', this.onInit = this.shaderInit.bind(this));
  this.shell.on('gl-render', this.onRender = this.render.bind(this));
  this.mesherPlugin.on('meshed', this.onMeshed = this.createBorderMesh.bind(this));
};

BorderPlugin.prototype.disable = function() {
  this.mesherPlugin.removeListener('meshed', this.onMeshed);
  this.shell.removeListener('gl-render', this.onRender);
  this.shell.removeListener('gl-init', this.onInit);
  this.shell.unbind('chunkborder');
  if (this.keysPlugin) this.keysPlugin.down.removeListener('chunkborder', this.onToggle);
};

This does have the downside the plugin needs to be responsible for cleaning itself up, but arguably if the plugin is registering event listeners, it should be the one to unregister them (single responsibility principle)

[rom1504]

Yeah that sounds like a good solution, that requires some more code in each plugin but it supports all kind of listening the plugin might want to do.

I'll try to see how that looks with some flying-squid (internal) plugins.

[rom1504]

Ah it does mean creating lot of onX functions though. Got to try to see if it doesn't clutter the code too much.