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

Have scales and plugins register themselves #6979

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jan 17, 2020

This should hopefully allow users to import just the scales that they need and allow the others to be tree-shaken out

We will need to do one of two things to really make this work:

  1. Create a controller registration service similar to the scale registration service instead of having users add to Chart.controllers as suggested in the docs
  2. Or stop using strings to specify chart and scale types and use the object types instead as suggested in [FEATURE] Charts as modules #4461. This would be more type safe and work better with tools like eslint, Typescript, etc. E.g. right now if you have a typo like you want to use the 'yime' scale instead of 'time' then you won't find out until runtime. But if instead you passed in YimeScale then that should be able to be caught at build time

These aren't necessarily mutually exclusive and we could possibly support them both, but I personally don't have time to do both and I think it would be potentially confusing to users to support multiple ways of creating charts (have to document everything twice, all the stackoverflow answers and docs look different, etc.). @etimberg @kurkle do you have a preference on approach here?

@kurkle
Copy link
Member

kurkle commented Jan 19, 2020

Tree shaking works with import / export ref counting.
So to make leafs drop, those must not be imported.

So I think it should be possible already to only import one scale and for example extend that.
We just need to explicitly import only the things needed, and not helpers/index.js for example.

Now, for extending a scale (for example), the self registering would be a bit annoying, wouldn't it?

I think the 2. is actually required for tree shaking.

Given that I'm quite a noob still in JS, anything I write should be considered with caution 🤣

@etimberg
Copy link
Member

I like the self registration. I'm a bit hesitant on replacing the strings, just because they are so easy to use. I'd be up to look at an implementation though to see how it could work and I'm happy to be proven wrong.

Re tree shaking, yes they need to be not imported. That will mean we have to get rid of all the Chart.* properties that expose the helpers, tooltip objects, etc. I'm not sure what that will do to our tests.

@benmccann
Copy link
Contributor Author

benmccann commented Jan 19, 2020

Now, for extending a scale (for example), the self registering would be a bit annoying, wouldn't it?

Why do you say that? The parent scale and child scale would both end up registered in that case, but I don't think there's any harm in registering a scale that's not used (we register all of them today). The parent scale won't be able to be tree shaken out anyway because it's imported by the child scale

I think the 2. is actually required for tree shaking.

I'm not sure I follow why you say that

Re tree shaking, yes they need to be not imported. That will mean we have to get rid of all the Chart.* properties that expose the helpers, tooltip objects, etc. I'm not sure what that will do to our tests.

I'm unclear as to whether we will need to do that. If the user just imports what they need instead of index.js I'm hoping it may be possible. But I'm not very confident on this point. You could very well be right on this

For the tests, it's not easy to import ../../src/helpers instead of using Chart.helpers since the core library gets compiled by rollup first. I tried removing the rollup step, which would fix #6754 as well, but one issue is that if you set type: 'module' in karma it expects all the import statements to be the filename (i.e. include .js at the end). Another option might be to either use rollup to compile the tests and library together

@kurkle
Copy link
Member

kurkle commented Jan 21, 2020

Now, for extending a scale (for example), the self registering would be a bit annoying, wouldn't it?

Why do you say that? The parent scale and child scale would both end up registered in that case, but I don't think there's any harm in registering a scale that's not used (we register all of them today). The parent scale won't be able to be tree shaken out anyway because it's imported by the child scale

Lets think of a awesomeScale that extends from any of the built in ones. This one was created 2 years ago and includes the version of build in scale at that date. When imported, the included 2 year old built in scale also registers, possibly overriding the current one. Can probably be gotten around by externals and peer dependencies and what not, but it can be annoying.

I think the 2. is actually required for tree shaking.

I'm not sure I follow why you say that

Not sure either 🤣 . The thought was 'time' is not anything that tree shaking can use, but import TimeScale from 'scale.time' is.

Example:

import {Chart, TimeScale, CategoryScale, Line} from 'chart.js';
new Chart(ctx, {type: Line, scales: {x: {type: TimeScale}, y: {type: CategoryScale}});

vs

import Chart from `chart.js`;
new Chart(ctx, {type: 'line', scales: {x: {type: 'time'}, y: {type: 'category'}});

First one does not actually need any registration, and 2nd one can not shake the other types.


Another option might be to either use rollup to compile the tests and library together

I think that could be the one.

@benmccann
Copy link
Contributor Author

benmccann commented Jan 21, 2020

Lets think of a awesomeScale that extends from any of the built in ones. This one was created 2 years ago and includes the version of build in scale at that date.

I'm not following. If it was built 2 years ago it would be using v2 and I don't think we should have to worry about mixing v2 and v3 scales in the same project. Or do you mean someone makes a version with v3 and then two years later makes another new version with v3?

When imported, the included 2 year old built in scale also registers, possibly overriding the current one.

Why would there be two different versions of the same scale in the project? That seems like it'd be a problem even today. How do you know you're getting a reference to the correct one to register?

First one does not actually need any registration, and 2nd one can not shake the other types.

Yeah, that sounds like my option 2 above. Are you suggesting you prefer that one?

@benmccann
Copy link
Contributor Author

Unfortunately it seems this won't work unless we figure out a way to remove the custom css rollup plugin

@benmccann
Copy link
Contributor Author

Adding a note here for posterity. The custom css rollup plugin has now been removed in master so this PR could be revisited if we decided on an API that looks like this

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

Successfully merging this pull request may close these issues.

None yet

3 participants