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

Generic registry for controllers, scales, elements and plugins #7435

Merged
merged 4 commits into from
Jul 6, 2020

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 28, 2020

Agrees with #7407 in many ways, only goes a lot further.

TODO:

  • make it possible to register defaults in root scope for core plugins
  • replace pluginService
  • handle core.scale defaults in shakeable way. Registry would currently put those in defaults.scales.scale, and that would be another breaking change.

Resolves: #7496

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks pretty good to me if it works, which always seems to be the tricky thing :-) did you test if things get shaken out in rollup and webpack?

docs/docs/developers/axes.md Outdated Show resolved Hide resolved
src/controllers/controller.bar.js Show resolved Hide resolved
src/core/core.register.js Outdated Show resolved Hide resolved
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction this is going. Looks like it includes #7412.

In terms of the changes of elements.Arc to elements.arc I kinda preferred the old version since they're classes. If we do make this change, we'll need to document it as a breaking change since these are publicly available.

@kurkle
Copy link
Member Author

kurkle commented May 30, 2020

I like the direction this is going. Looks like it includes #7412.

Yes, I included #7412 to make things a bit easier (I'm assuming it gets merged)

In terms of the changes of elements.Arc to elements.arc I kinda preferred the old version since they're classes. If we do make this change, we'll need to document it as a breaking change since these are publicly available.

I restored those to be capitalized.

etimberg
etimberg previously approved these changes Jun 14, 2020
@etimberg
Copy link
Member

Looks like this needs a rebase @kurkle
@benmccann have you had a chance to review?

@benmccann
Copy link
Contributor

This looks pretty good to me. I can do a final pass after rebase

@kurkle
Copy link
Member Author

kurkle commented Jun 28, 2020

Rebased. Had added a unregister (for plugins), which is now included.


const controllerDefaults = defaults[type];
const ControllerClass = registry.getController(type);
Object.assign(ControllerClass.prototype, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't we still set these in the controllers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't pull the element types with the controller. So when you derive from a controller, you can change the element types and the original ones can be shaked.
This also enabled changing the element type only, for example:

Chart.register(MyBar);
Chart.defaults.bar.dataElementType = 'MyBar'`;

etimberg
etimberg previously approved these changes Jul 6, 2020
@etimberg
Copy link
Member

etimberg commented Jul 6, 2020

Needs another rebase @kurkle.

benmccann
benmccann previously approved these changes Jul 6, 2020
@kurkle kurkle dismissed stale reviews from benmccann and etimberg via d45eb0f July 6, 2020 19:05
benmccann
benmccann previously approved these changes Jul 6, 2020
@kurkle
Copy link
Member Author

kurkle commented Jul 6, 2020

Noticed that scale service was mentioned in couple of places still, while it does not exist any more. So updated those.

@etimberg etimberg merged commit 6bd5ad5 into chartjs:master Jul 6, 2020
etimberg pushed a commit that referenced this pull request Sep 1, 2020
* Generic registry for controllers, scales, elements and plugins
* Remove references to scale service
@kurkle kurkle deleted the registry branch November 19, 2020 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not read sampleSize of undefined
3 participants