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

Discussion: should part of Chart Registry move to compat #1733

Open
kum-deepak opened this issue Aug 2, 2020 · 5 comments
Open

Discussion: should part of Chart Registry move to compat #1733

kum-deepak opened this issue Aug 2, 2020 · 5 comments
Labels
Milestone

Comments

@kum-deepak
Copy link
Collaborator

I see the following types of users:

  • Users that simply want to use dc to create a single group of linked charts, they would never create more than one group.
  • Users looking to create more than one group of linked charts on a single page.
  • SPA users, who would benefit from creating ChartGroup as a local variable.

Based on the above, we should do the following:

  • The recommended way of using ChartGroup/Registry.
  • Deprecate specific API and move those to the compat layer.
@kum-deepak kum-deepak added this to the dc-v5 milestone Aug 2, 2020
@gordonwoodhull
Copy link
Contributor

Could you spell this out some more? I’m missing the argument.

Also, what is “the specific API”?

@kum-deepak
Copy link
Collaborator Author

For example, if we suggest that users should always create an explicit ChartGroup object (i.e., empty or string argument is not allowed), then entire ChartRegistry and *All functions can move to the compat layer.

Another example, if an empty argument is allowed, but, string argument is not allowed (i.e., if other than default chart group is needed, an instance of ChartGroup must be provided), in that case, a significant part of ChartRegistry and most of *All can be moved to the compat layer.

I have my doubts originating from the fact that ChartRegistry maintains a global state. However, the updated stocks example does need a global state for this purpose.

This whole thing is optional - doing no change is acceptable as well.

@gordonwoodhull
Copy link
Contributor

Thanks! Now I get where you’re going with this.

This is good food for thought. I like the way it gets rid of global state, and I especially like the way that it gets us closer to #988, removing the chart registry in favor of something third-party. (We would have to deprecate the chart registry for a couple of major versions IMO.)

Let’s continue to think about this, and see what other global state there is.

As always, comments from the community are welcome!

@kum-deepak
Copy link
Collaborator Author

There are isolated variables here and there - like uniqueId keeps the last id, events trigger keeps the last event. However, I do not think anything major. I will keep looking closely.

@kum-deepak
Copy link
Collaborator Author

With the recent work with ChartGroup and FilterStorage, I believe we can now move the entire ChartRegistry to the compact layer.

For this, string argument for a group name will not be allowed. The default will be stored in the ChartGroup itself (created on demand). I will do a PR soon.

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

No branches or pull requests

2 participants