WIP: add registerController for controllers

[sgratzl]

implement one possible variant for registering controllers. whether the registerController method is wrapped with an generic register method is to be discussed.

The optional preRegister method is used to avoid using the .prototype statically which prevents tree shaking in my experiments. Similarly settings defaults by calling a method like merge will also prevent tree shaking.

To sum from my experiments:

class Test {}

// shakeable:
Test.id = 'A';
Test.defaults = { a: 5};
Test.preRegister = () => {...};

// not shakeable
Object.defineProperty(Test, 'id', 'A'); // uses Test thus not unused for removal
_defineProperty(Test, 'id', 'A'); // same and compiled from `static id = 'A'` by babel
Test.defaults = merge({}, [...]); // merge could have side effects
Test.prototype.dataElementType = Rectangle; // using a nested object

[benmccann]

Thanks for this!!

I'd also change core.controller to use the registry. Look for the line const ControllerClass = controllers[meta.type];

[benmccann]

I can't find anyone else online suggesting that prototype caused them an issue. Maybe babel is transforming it first which is causing issues? (e.g. Object.defineProperty(BarController.prototype, ... or something). Otherwise I think I'd report an issue to Rollup and see what they say before we introduce this workaround because it's a bit weird to me that this would cause trouble but BarController.id = 'bar'; wouldn't

[sgratzl]

take a look at

https://rollupjs.org/repl/

and you can play around yourself. btw. webpack is not better, in this simple test it not even shaked out the .id variant

[benmccann]

Should we move id inside preRegister as well then to help webpack?

[sgratzl]

then .preRegister still remains which has the same effect.

I don't know how to have something like .registerController(LineController) without having additional attributes like id, defaults, preRegister.

one could extract everything to a separate method: registerLineController but this be not as handy

[benmccann]

Good point. Thanks for that repl! It's very informative and helpful

I filed an issue with rollup to see if there's anything they can do on their side, but we should figure out what's possible just on our end in the meantime

I don't know how to have something like .registerController(LineController) without having additional attributes like id, defaults, preRegister.

What if we do something like:

export function line() {
  const defaults = { ... };
  registry.registerContoller('line', LineController, defaults);
}

Then users could do:

import { Chart, line, categoryScale, lineScale } from 'chart.js';
[ line, categoryScale, lineScale ].forEach(f => f());

[sgratzl]

I'd also change core.controller to use the registry. Look for the line const ControllerClass = controllers[meta.type];

I did change the import, so it should already use them.

[etimberg]

Could use beforeRegister and afterRegister? That way we'd match the format of other lifecycle calls

[kurkle]

I agree tho those names and we could have those hooks, but I think we should not expect anything to happen in beforeRegister/afterRegister. Maybe we can come up with a better name for this method. initializePrototype (just to get you thinking, I don't like that either)

[kurkle]

The id and defaults changes seem obvious and good.

[kurkle]

Somehow I don't like a function that modifies the prototype. But I don't really know why :)
Other option that comes to mind is return the things from a method or methods.
getDataElementType etc.

[kurkle]

I agree tho those names and we could have those hooks, but I think we should not expect anything to happen in beforeRegister/afterRegister. Maybe we can come up with a better name for this method. initializePrototype (just to get you thinking, I don't like that either)

[sgratzl]

seems like there are three main open points

how to deal with the .prototype.dataElementOptions and .prototype.dataElement?

1. keep it as it is either within the register method or outside of it and wait till rollup fixes the optimization problem.
2. go back to (static) class properties that rollup recently can proper tree-shake
3. switch to methods like getDataElementOptions and getDataElementConstructor
4. move the definition as part of the controller configuration defaults
5. extract the element definition / properties as static elements and make it part of the registered controller. add some utility functions to resolve them, like similar to getScaleDefaults

how to name the register method or the flow?

1. before/pre/prepareRegister or initialize which will be called as part of the registerController method, the use would be registerController(LineController) will call LineController.initialize
2. each controller has a .register method that will do the initialization and then call registerController

should controllers auto registers scales / [elements]

e.g., BarController will register LinearScale, CategoryScale, and Rectangle (if we wanna have it to register its defaults

pro: simpler configuration without the need to know details about the controller. e..g register(BarController) vs. register(BarController, LinearScale, CategoryScale, Rectangle)

con: in case the scale configuration is customized, e.g., using a time scale or logarithmic scale, the LinearScale is registered without needing to be.

[benmccann]

how to deal with the .prototype.dataElementOptions and .prototype.dataElement?

I got some tips from the rollup folks:

• It turns out that if we were creating classes the old-school way by just declaring a class by creating a function and then adding prototype to it then it'd work. Would this stop users from being able to do class MyScale extends Scale though?
• We could also create a pure annotated IIFE

We should probably test in Webpack as well, but I do see mention in their docs about the pure annotations, so it seems likely that would work

[benmccann]

should controllers auto registers scales

I'd vote no to this question for the reason you mentioned as the con. E.g. I most frequently use a TimeScale on the x-axis of a line chart, so would like to be able to be able to shake out the CategoryScale which is the default

[benmccann]

Rollup has a PR pending now to fix the tree shaking on their end without us having to do anything extra: rollup/rollup#3598. Anyone want to file a request with Webpack? :-)

[sgratzl]

closed in favor of #7435