Add new hooks for plugins

[kurkle]

Our internal plugins need to know when they are disabled.
This adds a install, uninstall, start and stop hooks.

install and uninstall are called on all plugins (disabled ones too)

[kurkle]

It would actually make the plugins a little cleaner if this was a new hook.

[etimberg]

I'm not opposed to making this a new hook. something like onStatusChange or split into two onDisable and onEnable

[kurkle]

We have reset, resize and destroy, so how about just disable? (and enable)

[simonbrunel]

IMO, a plugin should be either enabled or disabled for the whole lifetime of a chart. Allowing this state to be dynamic complexifies the plugin API for a few uncommon use cases. It also makes the whole hook logic less predictable: a plugin initially disabled will miss the before/afterInit hooks when re-enabled later, which are most of the time critical steps for the correct plugin initialization.

I initially suggested the shorthand config.options.plugins[id]: false to make easy removing a plugin for a specific chart but I don't think this should be supported dynamically. config.plugins may actually be a better place to support removing plugins somehow (and thus remove the false shorthand in config.options.plugins).

For the title & legend plugins, I think we should only support toggling plugins.title.display or plugins.legend.display if the user wants to do that dynamically (e.g. from a button). plugins.title: false would still work but would not react correctly if changed after the chart is initialized.

[kurkle]

So, we need to have consensus.

I can change this PR to only remove the tests (and fix the cc issue), if that is how we want it to be. We have the display = false way of hiding the plugins already.

I just don't want to leave the disabled tests around.

Another thing that should be considered, is that users can now add/remove plugins after chart is initialized. We should be throwing errors if that is not supported.

Here is a pen demonstrating some aspects of the current behavior: https://codepen.io/kurkle/pen/ExgYJoj

[simonbrunel]

Another thing that should be considered, is that users can now add/remove plugins after chart is initialized.

What's the use case for this?

[kurkle]

What's the use case for this?

IDK. But its currently possible.

[simonbrunel]

I don't think of any use case where a user wants a plugin to be dynamically added or removed to a chart already initialized. Maybe there is but then I would wait this request to be reported with a good rational before considering adding complexity on the API (since it may not be used anyway or misused).

[kurkle]

IMO the current behavior is not acceptable.

We moved options.legend to options.plugins.legend (same for title and tooltip). But they don't behave the same.
options.legend: false is such a common use case, that I'm sure there are users who set it dynamically.

I see at least these ways of fixing it:

• make the option work as it used to, in its new location
• move the option back, so it works as it used to
• document the new behavior, instruct using options.plugins.legend.display=false instead, in addition throw an error to make it obvious (would be pain to debug)

For me, the first one is really the only option. Achieving that, there are at least couple of ways:

• this current one or some variant of it: let the plugin know when its disabled
• make the element, that is no longer removed by the plugin, check if it is in fact disabled
• make exceptions in plugin service for these plugins, and keep notifying them even when disabled

This one that I proposed is the only one that is also an enhancement.

@simonbrunel are there any options that you can agree on?

[simonbrunel]

But they don't behave the same.

That's a new major version, does it really matter? Previously, setting false didn't disable the plugin, so it was more comparable to a shorthand for title.display: false.

I'm sure there are users who set it dynamically

I'm sure they do, but with the intention to hide title/legend, not to explicitly disable the plugin, so title.display is totally fine.

This one that I proposed is the only one that is also an enhancement.

I don't see this is an enhancement, apart being consistent with v2 and still support the option shorthand (which is not a good reason). Did we get this request (I didn't check existing tickets, so maybe yes)? I'm not convinced it will make the plugin API more robust: as I mentioned, there are a few cases that doesn't work well with this disable state (e.g. beforeInit/afterInit).

Are there any options that you can agree on?

The state of dynamically displaying / hiding something (title, legend, datalabels, etc.) is the responsibility of the plugin, thus this option should be provided by the plugin. Trying to make the plugin API more complex to handle it, while there is already simple solution, doesn't make sense to me.

Option 4:

Another option not listed in your previous comment could be to never disable a plugin when options.plugins[id]: false. That means a plugin would be called on all hooks with false as options, so each plugin can react to this change, dynamically. This would allow an initially disabled plugin to still be able to handle beforeInit/afterInit for internal initialization.

[kurkle]

Comments just about option 4:

• beforeInit/afterInit would still not be called if the plugin is added after initialization. Remove the config.plugins array too? Though I think you can still use Chart.register after some charts are initialized. An instance count check there would prevent that. But is this not making things more complicated from every point of view, except the plugin hook call order? Is the call order actually something that is guaranteed? If it is, then I finally understand your point.
• possible performance problems with event handlers and globally registered plugins that you'd like to disable for one chart.

Anyway, changing how the whole plugins API works because we moved the internal plugin options to be consistent, does not seem like the right decision.

[simonbrunel]

beforeInit/afterInit would still not be called if the plugin is added after initialization...

Option 4 comes with my first remark that a plugin should be either registered or not for the whole lifetime of a chart. I would not remove config.plugins but extend it to be able to unregister locally a global plugin. When creating a chart, we calculate which plugins are enabled for this chart and that never change afterward.

possible performance problems with event handlers

I don't think there will be performance hit (you don't have 100 plugins registered on a single chart, but even, would it be an issue?).

Anyway, changing how the whole plugins API works ... does not seem like the right decision

Actually, I don't even like the current behavior (v2) of being able to dynamically enable/disable (add/remove) a plugin after the chart is created (e.g. on config update) because a plugin can't know that it missed some important hooks, for example:

• initially disabled plugin will not have its internal state initialized because it missed the init hooks
• initially enabled plugin will not be able to cleanup internals if disabled while the chart is destroyed
• will not be able to sync with the chart state change while being disabled (e.g. resize)

I'm not sure I would go for option 4 but I find it quite interesting because it gives all the flexibility to the plugin on how to handle its disabled state. It also simplifies and makes more consistent/predictable the plugin logic.

[simonbrunel]

I still see an issue with the beforeInit hook, which exists because some plugins must do some logic before the chart is initialised. If this plugin is initially disabled and enabled later, it will be called on init but after the chart is initialised, which would be too late for the plugin to correctly work.

[simonbrunel]

One way to make this case better would be to always call init before any other hooks, whatever the plugin state. If the plugin receive options: false to this init hook, it would know that it is initially disabled.

[simonbrunel]

For the hook names, I would suggest:

• install/uninstall: init is too confusing with beforeInit/afterInit. Also "install" is quite common in the plugin world and I feel it translates better the goal of this hook: "Install yourself on this chart".
• enable/disable: for consistency with other hook names and because enabled/disabled are more state names, not actions.

[simonbrunel]

I'm wondering if it makes sense to call enable if the state didn't change because the plugin already knows from install that it's initially enabled/disabled. Would you call disable here as well if the plugin is initially disabled?

Documentation for enable would be: "called when the plugin state changes from disabled to enabled" and vice-versa for disable.

What do you think?

[kurkle]

Thats actually why I called it enabled
If you disconnect enabled/disable from install/uninstall, it makes sense. And disconnecting those makes sense for simpler plugin state management.

Essentially avoiding the need for same code in install/enable in a plugin that does not care about the Chart init or destroy

[simonbrunel]

This is debatable since it's easy to refactor common code between install/enable and call a single function from both. But you may also want a different logic when initially enabled vs the state changes from disabled to enabled, which is currently harder to detect.

I don't think the name enabled makes it more obvious: It would be onEnabled which I think means that the state has effectively changed (similar to the onStateChanged suggested by @etimberg).

If we keep calling enable here, then we also need to call disable for consistency if the plugin is initially disabled.

[kurkle]

IMO one of the important goals of a plugin interface is to make things easy and simple. The proposed way supports 2 new ways of "state handling" without actual state handling in the plugin.

But you may also want a different logic when initially enabled vs the state changes from disabled to enabled, which is currently harder to detect.

I don't think there are any existing plugins that do such a thing (because it is impossible to detect disabling before this).
If you need to detect the state change, it means you have some internal state handling. Thus, how is it harder to detect?

{
  install(chart, args, options) {
    // always called
    // state = options !== false
  },
  beforeInit(chart) {
    // only called if enabled
  },
  enable(chart) {
    // only called if enabled
    // changed = state === false
    // state = true
  },
  disable(chart) {
    // only called if enabled
    // state = false
  },
  destroy(chart) {
    // only called if enabled
  },
  uninstall(chart) {
    // always called
  }
}

Can you describe a use case that is harder to do with these vs omitting the first enabled call on an initially enabled plugin?

[simonbrunel]

Ok, I think you are right, maybe start/stop would be better names than enable/disable then?

When I read enable, I expect it to be called if the previous state was "disabled" (which is not necessary the case after install). But I would expect start to be called after install if the plugin is initially enabled or has been re-enabled.

install > start (if enabled) > stop (if previously started) > start > uninstall

[kurkle]

@etimberg's suggestion with the on prefix actually make sense too, because these are plugin events while the others are chart events.

[simonbrunel]

on prefix would make sense for all other hooks because these are events (e.g. onBefore(Chart)Init, onBefore(Chart)Update, on(Chart)Destroy, etc.). I don't think install/uninstall, enable/disable should be prefixed, these are more actions on the plugin itself instead of events.

[kurkle]

on prefix would make sense for all other hooks because these are events (e.g. onBefore(Chart)Init, onBefore(Chart)Update, on(Chart)Destroy, etc.). I don't think install/uninstall, enable/disable should be prefixed, these are more actions on the plugin itself instead of events.

True, I was actually just trying to state that differentiating the names would make sense. I'm a bit opposed on renaming the old hooks though (because it would break every single plugin)

[etimberg]

install, start, stop, uninstall make sense to me. Using onEnabled to me only makes sense if we call it at a state change. It's a bit unintuitive to have it sent on create, but start makes that more clear.

[kurkle]

Updated. So, one more thing. Do we want to have a stop on chart destroy?

[simonbrunel]

It would make sense yes, just after destroy and before uninstall (and only if the plugin is enabled/started).

[simonbrunel]

Looks good, thank you @kurkle!