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

Looking towards next major release #1692

Open
kum-deepak opened this issue Jun 29, 2020 · 6 comments
Open

Looking towards next major release #1692

kum-deepak opened this issue Jun 29, 2020 · 6 comments
Labels
Milestone

Comments

@kum-deepak
Copy link
Collaborator

This library is popular, works well, and has been in production use for many years. The two previous versions focused on upgrading to the latest d3 and converting it to ES6. In the process, quite a lot of tooling got upgraded as well. All these facilitate further refactoring to be easier.

Key guiding principles:

  • No existing functionality should be dropped.
  • Some of the syntaxes might change - however, compatibility should be maintained as far as possible.
  • The newer syntax should not be more cumbersome than the existing syntax.
  • Documentation should go better.

We ourselves want to expand our usage of this library. For that we will like to make the following simpler:

  • Using the library with a remote data source.
  • Saving and restoring filter states for the dashboard.
  • Using a single standalone chart without crossfilter.
  • Adding new chart types.

I see separating concerns as the key in achieving the goals:

  • Data sources
  • Managing filters
  • Capping
  • Ordering
  • Existing Mix-ins
  • Chart rendering
  • Chart redrawing

My PoC in #1665 makes me believe that quite a lot of it is feasible.

An interesting case will be to see if the Row and the Pie charts can share more.

Some items that I think can be taken up:

  • Moving to Typescript - this will help us in improving code quality (type checking, public/protected/private members). This library will be inputs that are not simple values or functions (like an Object with several methods) - the Typescript Interface definition can act as good documentation. With >ES6 as the output target, the output will almost be like the current ES6 code. Source maps keep the debugging the browser against the original code possible.

  • Allow saving and restoring filter state (to from JSON probably).

  • Quite often there is a valid case of using the same dimension in more than one chart. Currently, the filter state is maintained at the chart level but applied to the dimension - which causes issues. It seems possible to maintain it at the dimension level.

  • Chart Group can become a class so that each of the Chart Group is an instance.

  • Split Filters into independent classes, inheriting from a Common Interface.

  • Consider creating a simple filter class that implements the Filter interface, consider creating a filter list class, which itself implements Filter interface.

  • Consider making all util functions as top-level functions - bundlers are going to be happier. Remove usage of pluck.

  • Reconsider filter printers.

  • Depending on how much compatibility is required - compat code can be moved outside and be applied on top of the new code. We might consider creating two bundles - one only with updated API and one with compatibility API.

I am reasonably clear on each of the individual tasks. When we reach an agreement I can start work.

@gordonwoodhull
Copy link
Contributor

I agree with the goals and separation of concerns. (I guess you mean that rendering and redrawing should be one concern, not two. Ideally, I guess those would be the only concern of the chart classes.)

I'm not sure what you mean about shared code between the Pie and Row charts. If anything, the thing people ask for most is for the Row chart to become Stacked. Do you mean that more of the Capped functionality could be factored out of them?

I partly agree that transpilation from Typescript is OK in 2020, especially now that we have ditched IE. Sourcemaps still don't work perfectly in Firefox or Safari, and being able to debug across all modern browsers is extremely important. So I am wary here, but supportive.

The reuse of dimension, i.e. sync of filter between charts, is discussed in #595 and #682, and I had some success with an implementation in the sync-filter branch. The interface was simple, a string filterGroup for each chart that works analogous to a chart group. Any charts which specified the same name would be sync'd, if unspecified or different they remained independent. Maybe if you are separating concerns the state could be shared rather than broadcast.

I am worried that our test coverage, although pretty great, might not catch every way that charts are used. So I urge you to also run through the examples every once in a while when you are refactoring, and make sure they still work or can be fixed.

The recent example of ordering being broken for ordinal keys was a good example. There was a test that should have caught this, but it succeeded by accident. However, one of the transitions examples was obviously broken. I don't know any good way to test animated transitions, so these examples are visual tests that need a human in the loop.

(Many of our transitions don't quite make sense, but the pages should still work.)

I need to do another review of #1665 to make sure I agree with the approach and that it simplifies and doesn't complicate things. But overall I am 👍 on this, and I think you should go ahead!

Do you have any examples of how the syntax might change?

@kum-deepak
Copy link
Collaborator Author

Thanks for all your inputs. I will start by creating a branch, milestone, and label dc-v5.

The first work will be convert to TypeScript which will help later part of the work. I will like to include #1669 before I start work.

If required I can be avaialble on slack or a Skype call to speed up your review of #1669.

@kum-deepak kum-deepak added this to the dc-v5 milestone Jun 30, 2020
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jun 30, 2020

Are you asking about #1665 or #1669? I am currently reviewing #1665 which seems to include #1669 as well.

Thanks for offering to meet, however I'd rather keep our conversation documented on GitHub, since I rely on GH for my memory. 😄

@kum-deepak
Copy link
Collaborator Author

I did mean #1669. This merged with the current develop should be treated as the base for dc-v5. #1665 was a PoC for me, which has served its purpose. That work would be redone based on the learnings.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jun 30, 2020

Okay, I'm just going to have to trust you about #1669. The way getColor worked with this is such a dreadful contraption, it's really difficult to understand all the implications of the old and new behavior.

What I will do is review #1669 before we release v5 and at that point we can decide what needs to go into the upgrade guide. I am sure it is going to break someone's customizations somewhere.

I have also run through the relevant examples and didn't see any issues. But I don't think we have enough tests for color customization. We don't even have an example of a bar chart where all the bars are different colors (the way the row chart works). That's pretty common - iiuc it would not be affected by #1669 since that change mostly affects layers.

Talking about it doesn't help, I just need to spend a half a day staring into the horror.

If we do need to restore backward compatibility, it will be easier to do that on top of code that makes sense, so it's enough to preserve those changes for reference. (Please don't delete that color-accessor branch!)

@kum-deepak
Copy link
Collaborator Author

All of the above makes sense. I have gone through the horror that you mention. It took me more than a day to undtangle by stepping through the debugger using different examples 😄

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