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

v3: controller, element, scale, plugin and defaults registration #7403

Closed
kurkle opened this issue May 22, 2020 · 6 comments
Closed

v3: controller, element, scale, plugin and defaults registration #7403

kurkle opened this issue May 22, 2020 · 6 comments

Comments

@kurkle
Copy link
Member

kurkle commented May 22, 2020

Originally posted by @sgratzl in #7371 (comment)

There was a PR already in January: #7009

  • IMO we should replace the scaleService with registry (insert better name here), that would handle all of these registrations.
  • it should be a singleton
  • it would be exposed through chart as a static method

It would be used like:

import {Chart} from 'chart.js';
import AwesomeController from .'/controller';
import AwesomeElement from '/element';
import AwesomePlugin from '/plugin';

Chart.register(AwesomeController, AwesomeElement, AwesomePlugin, /*AwesomeDateAdapter, AwesomeColorLibrary*/);

This register would store the constructor by type in a map, set defaults etc.
The type (controller/element/plugin) would be determined from the prototype.

@sgratzl
Copy link
Contributor

sgratzl commented May 22, 2020

in my plugins I used a different approach: not a central register method but each element had a register method.

as in https://github.com/sgratzl/chartjs-chart-graph/blob/21bdfe04b132bbaf45506eb0fea8515895d25433/src/controllers/graph.js#L396.

The advantages were that I could add some custom logic in each method. like registering other components or elements or compute the defaults on the fly for better tree shaking.

Moreover, why not a flat register method instead of a static property?

@benmccann
Copy link
Contributor

I don't have any objection to adding a register method to each element. I think to make it easy for users it would still be nice to have a Chart.register that would call the register method on elements passed into it. Those elements would then have to add themselves to a registry where we could look up the element by id

@etimberg
Copy link
Member

Catching up on all the conversation around this. I don't know that having a single register method that inspects types would be a good idea. Right now, for example, we don't enforce date adapters to extend from the DateAdapter class (exposed as Chart._adapters.date) and we've market the class in a private namespace.

I could imagine something like this

Chart.registry.addScale(scale);
Chart.registry.addController();
Chart.registry.addPlugin(..);

One thing that might be nice in a debug build using this system (if we can statically compile it out) would be to verify that the registered objects match the expected prototype).

@benmccann
Copy link
Contributor

Date adapters don't have to be registered because they're not looked up by id. There's only a single instance of a data adapter. We could make them extend DateAdapter and probably should make _adapters public, but I'm not sure it matters for this discussion

The thing I don't like about registering independently is it's verbose and you'll have to do it for every project

Chart.registry.addScale(timeScale);
Chart.registry.addScale(lineScale);
Chart.registry.addController(lineController);

Compared to:

Chart.register(timeScale, lineScale, lineController);

We could have the typed register methods (addScale, addController) that could be called by the more general one. That would also allow users the ability to register anything that's not extending from a class still

@sgratzl
Copy link
Contributor

sgratzl commented May 23, 2020

I started creating a simple registry for controllers in #7407

@benmccann
Copy link
Contributor

Closing this since we have a registry now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants