Charts to ES6 classes #1561

kum-deepak · 2019-08-18T12:51:33Z

Have been experimenting with multiple approaches so far. At this stage trying to figure out what the final ES6 classes should look like and also how to get there without, potentially, introducing errors.

kum-deepak · 2019-08-18T13:14:44Z

I am trying to come up with a guideline on how an ES6 class should look in dc. Please see https://github.com/kum-deepak/dc.js/blob/charts/src/base/legend.js

So far the guidelines look like the following:

All variables with names starting with _ be converted to fields.
In ES6 declaring fields are not supported yet (https://github.com/tc39/proposal-class-fields)
To make it explicit, all fields be initialized in the constructor.
File specific constants (all CAPTIAL_CASE_ONES) be placed before the class declaration.
All named functions be promoted to member functions.
Private member function names must start with _.
There are cases where, typically an anonymous function,
this is used based on a future binding. In those cases
self be used for current this in smaller context.
See: https://github.com/kum-deepak/dc.js/blob/charts/src/base/legend.js#L268
for an example.

Looking for you feedback/suggestions.

gordonwoodhull · 2019-08-18T15:17:57Z

This makes sense. I'll review the Legend class more closely tomorrow.

I didn't think about the lack of private members in JS. That's a big change. I guess there is a proposal (with weird hash syntax) which is making its way to browsers, but not fast enough for us. (Same document you linked)

I think this is okay. dc.js was intended to be a leaky abstraction, and there are times where it's difficult to implement a special behavior or a workaround because the members are inaccessible.

We've been inconsistent about using _chart (in this example _legend) vs this. Originally it was always _chart but this has crept in because it usually works. Now with classes it will always be this, but as you say, occasionally self will be needed.

Personally I would advocate for the most terse accessor syntax possible:

arrow function if d3's this-element binding is not needed, allowing use of this to refer to the chart object.
old style function() where d3 this-element is needed (should be rare)
old-style function(), with explicit self binding to the chart object, as the legend example you linked, when both element and chart objects are needed. (should be very rare)

I recognize it's not consistent that the chart is usually this but sometimes self. But I bet accessing the chart is 10x as common as accessing the element - the only times I can recall using the element in an accessor are when I need the element's (text) size. Although there are probably esoteric uses I'm forgetting.

I'm open to debate.

kum-deepak · 2019-08-18T15:40:45Z

Yes, the lack of private members and no ability to explicitly declare members was something that stumped me. However with the current naming convention, in future, it should be easy to switch to the #s.

Totally agree about the usage => functions. I am updating the guidelines.

So far the guidelines look like the following:

All variables with names starting with _ be converted to fields.
In ES6 declaring fields are not supported yet (https://github.com/tc39/proposal-class-fields)
To make it explicit, all fields be initialized in the constructor.
File specific constants (all CAPTIAL_CASE_ONES) be placed before the class declaration.
All named functions be promoted to member functions.
Private member function names must start with _.
The tersest function syntax be used:
- arrow function if d3's this-element binding is not needed, allowing use of this to refer to the chart object.
- old style function() where d3 this-element is needed (should be rare).
- old-style function(), with explicit self binding to the chart object, as the legend example you linked, when both element and chart objects are needed. (should be very rare), see: https://github.com/kum-deepak/dc.js/blob/charts/src/base/legend.js#L268
for an example.

gordonwoodhull · 2019-08-19T17:06:06Z

Wait, I thought you were going to use ES6 class member functions instead of initializing all the members in the constructor?

I appreciate that this is closer to the original implementation and semantics, but we won't get any of the performance benefits this way (every function gets created every time a class is instantiated) and it's not idiomatic.

Are there problems with class members?

gordonwoodhull · 2019-08-19T17:24:18Z

Whoops never mind, I missed a commit there!

I see what you are doing now.

This looks really nice. 👍

kum-deepak · 2019-08-20T01:27:18Z

After several attempts, baseMixin has been undergone basic conversion. Beyond my expectations, all test cases pass and examples seem to work. 👍

Additional learning:

Any member that gets overridden may cause attribute name conflict. For example, group when overridden the parent method is renamed to _group. While _group has now become a private attribute in the parent. To avoid such conflicts, the _group attribute in parent is renamed to _pvt_group. These will be renamed back after the full conversion.
logger.deprecate need to be replaced with another approach. Currently any such method is placed at the end of the constructor.
It seemed to me that there are many functions that are held by attributes allowing these to get reassigned on an instance by instance basis. Currently BaseMixin's constructor initializes these. Please review these.
Functions _defaultWidthCalc and _defaultHeightCalc - the current logic does not allow these to relocate. These have been left where these were.

Moving to to other classes now.

gordonwoodhull · 2019-08-20T01:56:06Z

Cool!!

We probably don't need dc.override anymore since ES6 classes have a nicer way to override methods in a derived class. This is another case where prototype inheritance really is better than the Crockford-style closures and objects we had.

That should clear up the first issue, which sounds painful.

As for all the functions initialized in the constructor: since none of those depend on the scope, the bigger function definitions could be moved out of the class and be private to the module. They'd just be referenced in the constructor, with less clutter and maybe a little more clarity.

I agree about needing a new deprecation logger. I'll think about it.

kum-deepak · 2019-08-20T03:37:20Z

Thanks @gordonwoodhull. dc.override will be removed when the first round of conversion is complete.

Just curious, should we make conventions for ordering of member functions (and constructor)?

kum-deepak · 2019-08-20T04:19:34Z

Took a shot at moving initializer function as private methods. Currently placed towards end of the class.

gordonwoodhull · 2019-08-20T12:37:49Z

I meant private to the module - at file scope. None of them use this or anything from the class, so I think it would be clearest to declare them as independent functions.

They don't really make sense as member functions, for the same reason.

And if they are not exported, they won't be visible or cloud up any namespaces.

gordonwoodhull · 2019-08-20T12:47:55Z

And sure, if you see a consistent ordering for class members that works for chart classes, that would be helpful. They are a jumble right now.

The categories that come to mind are constructor, getters/setters, public methods, private methods and utilities. (It also helps to make a distinction between truly private methods and internal interfaces like doRender. But there is no list or documentation currently.)

I'll leave it to your judgment what order to put them in. There are good arguments either way.

gordonwoodhull · 2019-08-20T12:49:15Z

Also, looks like you need to rebase - I made a minor doc fix for the crossfilter PR.

kum-deepak · 2019-08-20T14:37:48Z

Code has changed a lot, unfortunately rebasing is no longer possible. I have checked the diff, I will just make the corresponding change.

gordonwoodhull · 2019-08-20T16:55:38Z

With respect to method orders, I guess the counterargument is that it's often nice to have helper/utility functions near the public or "internal interface" methods they support, grouping code by functionality rather than what category it's in. E.g. "all of the brushing code", "all of the legendable code", drawing functions, etc.

Could this subjective factor be addressed in the guideline?

Another consideration is that many of the utility functions don't use this and were only inside classes in order to hide them. With modules, that's not necessary. (Just like the function defaults we discussed above.)

kum-deepak · 2019-08-22T04:09:42Z

Makes sense. At this point I will focus on conversion of classes. We can take up reordering of methods later if we feel like it 😄

gordonwoodhull · 2019-08-24T22:42:14Z

Looking good! 👍

gordonwoodhull · 2019-08-25T19:46:25Z

_tweenPie doesn't use this to refer to the class object, and no one ever asked for the ability to override it, so I'd argue it also belongs in the file/module scope.

I'd declare it like this:

const _tweenPie = self => function(b) {
// ...

and use it like this:

tranNodes.attrTween('d', _tweenPie(this));

kum-deepak · 2019-08-25T19:49:01Z

That looks like a good idea, I will apply similar pattern to sunburst as well.

kum-deepak · 2019-08-25T20:17:04Z

Please review the change set. I also needed to explicitly set this at invocation. Do you think there is a simpler way?

All test cases pass and tween seem to be working.

At top level this is undefined, so rollup warns. So, instead of => used simple function.

gordonwoodhull · 2019-08-25T20:37:41Z

Cool! I'll take a thorough look at all the thiss and selfs tomorrow.

Yes arrow functions do not have their own this so they can't be bound or have this apply'd or call'd like d3 does. Hopefully there's a really simple pattern we can follow.

gordonwoodhull · 2019-08-26T17:05:57Z

Thanks @kum-deepak! I took a closer look at the BaseMixin Legend, PieChart and SunburtChart.

Excellent work - the "real" classes are much clearer than the Crockford-style class-closures we had before. Overall this is very clean.

"private methods" of classes

Previously there were free-floating functions inside of the dc.js classes, some of which accessed the current state through the closure, and some which didn't.

The functions which don't access the state can be put in the module, as you implemented for the base mixin. (Thanks!)

The ones that do access state need to be made members, but I would suggest one small change: since they were previously private, they should have the underscore prefix. This is consistent with "internal" methods like _doRedraw, and suggests to users that it won't be useful to call these methods.

I am thinking of methods like the base mixin's sizeSvg and generateSvg, checkForMandatoryAttributes.

Looks like you already did this for the pie chart's drawChart, createElements, createLabels, etc., so I'm glad we're on the same page!

A small point, open to debate: the base mixin's _defaultData could be at module scope - it doesn't use the chart object or closure. This wouldn't stop derived classes from overriding .data() with function which does use this - as the stack mixin does.

_tweenPie

I agree that there needs to be a function() (and not an arrow function) in order for this to be bound. But I tested

const _tweenPie = chart => function(b) {
    // ...
};

        if (tranNodes.attrTween)
            tranNodes.attrTween('d', _tweenPie(this));

and it works. I find it simpler than _tweenPie.call(this, d, self), along with having to declare self. But I know that tastes differ!

It looks like the same approach could work for the sunburst's tweenSlice and onClick. (Note that we use onClick and this.onClick inconsistently in the sunburst chart. The latter is correct because it is overridable.)

I like the parameter name chart: it's more descriptive than self.

_pvt_onClick

These make my brain hurt. Hopefully they will get a lot better with regular ES6 inheritance and overrides!

I did not look deep into every method implementation but I looked at a few dozen of them. Overall I think these changes make the code easier to read.

If there are any places where you think the transformation makes the code more complicated, please reach out.

kum-deepak · 2019-08-26T21:18:45Z

Thanks @gordonwoodhull!

I am prefixing _, whenever I realize the method/member is actually private. I have a sense that there may be a few opposite cases. Once full code has been converted to proper classes, IDE capabilities allow easier refactoring, including renames.

I am currently intending to keep all charts working and test cases pass, at least as far I can keep. This is helping me in avoiding errors.

The pvt is marker for me to come back.

gordonwoodhull · 2019-08-27T02:11:28Z

That sounds like a great plan.

I also like keeping everything running while refactoring. It may end up being more work, measured in characters added/deleted, than making N changes at once. But you don't end up in that state where nothing works and you don't know why. (which I know too well!)

gordonwoodhull · 2019-09-03T14:35:21Z

This is a monumental task you've taken on!

It's looking great so far.

kum-deepak · 2019-09-09T09:17:32Z

Completed one pass on all charts. Finally came at the other end.

Next I will do some Gruntfile changes before hitting mixins. At that stage will make another pass at all the charts.

gordonwoodhull · 2019-09-09T12:19:27Z

Excellent work! The incremental approach makes so much sense for such a complete and thorough change.

I took a peek at one of these, the composite chart. I forgot that we were deleting yAxisMin and yAxisMax - what a strange thing to do! This transformation will expose a lot of strange practices.

I think you are using self in a few places where it's not needed - this will be available unless there is an intervening function () in the scope. As always, we can revisit this in a later iteration.

Thanks for taking this on, @kum-deepak!

kum-deepak · 2019-09-09T14:29:42Z

There are few places where a method is deleted, my current thinking is to either raise an exception or just an error is logged (which I found in some cases).

Currently I have put ES6 as a marker for me to revisit.

Currently, I have been using a set of regular expressions and some patterns to figure out what all can be relatively safely changed. I am keeping self as a marker to indicate that I have to come back. Ultimately where it can not be removed, I was thinking to eventually rename it to chart where it can not be eliminated.

gordonwoodhull · 2019-09-09T14:43:23Z

I see, deletion is used in the composite chart in order to make sure the wrong implementation of yAxisMin and Max are not called. I agree that an exception might be better here.

I thought self might be a product of automatic translation - thanks for confirming. That makes sense.

kum-deepak · 2019-09-09T15:23:08Z

Updated the last commit, the automated system removed all of the self in composite chart.

kum-deepak · 2019-09-09T21:47:17Z

Spoke too soon, bar chart seems yet to be converted.

gordonwoodhull · 2019-09-10T14:44:41Z

Re browserify, just to document this while I'm looking at it, in case anyone comes searching later.

In the past we were generating a browserify bundle in order to test that our UMD code worked correctly with that bundler. Looks like we disabled it in beef2c8 about a year ago during a grunt cleanup in support of Karma.

Arguably it could still have some use in dc.js 3.0 but I agree it should be removed for dc.js 4.0. Browserify sort of does support ES6 modules (browserify/browserify#1186), but the point of this test was to make sure that our UMD code worked correctly.

Rollup will generate our UMD going forward.

gordonwoodhull · 2019-09-11T16:55:12Z

I am releasing 3.1.3 with the canvas scatter plot.

@kum-deepak, up to you if you want to pull these changes into the es6 branch or leave that to me. I've also been updating examples every once in a while, so let's sync up when you're done with the current stage of the transformation.

kum-deepak · 2019-09-11T21:41:00Z

We can sync examples and specs easily. However, merging any file in src is not going to be trivial at this stage.

gordonwoodhull · 2019-09-11T21:50:25Z

It took a little thought rebasing the scatter canvas code from DCv3 to DCv4, but it's fairly straightforward - a couple of new functions. I'll take care of it when we merge this PR.

kum-deepak · 2019-09-11T21:54:27Z

Thanks @gordonwoodhull!

kum-deepak · 2019-09-14T16:58:33Z

Learned that stack based charts work quite differently than other charts. Been able to resolve most of it, barring their usage of getColor. I have created a separate issue to track this one (#1575).

gordonwoodhull · 2019-09-16T11:14:43Z

I took a quick run through the examples and tests to see how this is going.

I take it you haven't needed to make any breaking changes to the interface so far? I am impressed but also surprised.

kum-deepak · 2019-09-16T12:00:22Z

I am surprised as well.

I am really thankful to all predecessors who have meticulously written test cases. When anything went wrong at least one of the test cases would fail - making the work easier. So, it has been a slow effort but not frustrating which earlier I was afraid about.

gordonwoodhull · 2019-09-16T19:14:22Z

That's great to hear.

It is a ton of work to write and maintain tests, often more work than writing features. TBH this is the main reason so many PRs don't get merged. It's hard to convince people to learn how to write them, and I can't always take the time myself.

But they make changes so much safer.

I'm sure there will still be glitches, since people use this library in lots of weird ways. But it's very comforting to see the tests and examples continue to run, with almost no changes.

kum-deepak · 2019-09-19T04:14:35Z

I think we have reached a stage to merge it to ES6 branch. All tests pass and eslint clean. Time for reviews 😄

We can merge newer examples and other changes at this stage.

Also time to plan and start conceptual changes.

gordonwoodhull · 2019-09-19T14:13:48Z

The code looks beautiful! It doesn't fix bugs in itself, but I think having such orderly code will help maintenance a lot going forward.

~~It will take me a little time to review and I'm not sure if I have that time today. I want to go over all the places that are commented ES6.~~ (not necessary to deal with this before merging to the es6 branch) Hopefully we can find some resolution for all of those before we release.

gordonwoodhull · 2019-09-19T14:15:28Z

I did notice a few places, notably in cbox-menu and series-chart, where

const chart = this;

is not strictly necessary. It's not a big deal, but could you do a grep for chart = this (19 instances) and review these again? I agree almost all of these are necessary but it looks like a few of these slipped through the cracks.

gordonwoodhull · 2019-09-19T14:24:28Z

I'm inclined to merge to es6 after dealing with those glitches. We can call it alpha 2!

Do you have open ES6 issues/questions which are not commented ES6 in the code?

gordonwoodhull · 2019-09-19T15:09:12Z

src/charts/series-chart.js

                    .brushOn(false);
            });
+        // ES6: do we add removal function in composite chart?


Although it's a great idea, it's not really an ES6 issue, so let's deal with it later.

As always, adding features requires thinking carefully about impact, as well as tedious but valuable test-writing.

For now, I don't think chart is necessary here. (much easier discussion!)

I agree, let us drop this.

kum-deepak · 2019-09-19T17:31:50Z

I'm inclined to merge to es6 after dealing with those glitches. We can call it alpha 2!

Do you have open ES6 issues/questions which are not commented ES6 in the code?

There is still work before I would suggest to mark another alpha. For me a key benefit merging to ES6 branch brings is that I can work on multiple PRs in parallel. In this instance I had to work quite long in a linear sequence.

In particular I want to clean up files in core folder. I will do those as smaller PRs.

kum-deepak · 2019-09-19T17:32:22Z

I did notice a few places, notably in cbox-menu and series-chart, where
const chart = this;
is not strictly necessary. It's not a big deal, but could you do a grep for chart = this (19 instances) and review these again? I agree almost all of these are necessary but it looks like a few of these slipped through the cracks.

Let me have a look 😄

gordonwoodhull · 2019-09-19T17:57:02Z

Thanks @kum-deepak. My idea is to bump the version number when I merge to the es6 branch, but not actually release the library to NPM or advertise it until it goes to master.

If anyone wants to try out the bleeding edge, they should be able to. Bumping the version number is a precaution in case weird versions of dc.js get out on the web.

kum-deepak · 2019-09-19T18:04:12Z

Makes sense. Please do it.

gordonwoodhull · 2019-09-19T18:57:25Z

ok github, yes you are correct, this is merged, but i'm still working on integrating the changes from develop/master, so don't get too excited!

Bugfix in canvasElementSize calculations and add documentation Update examples squashed from 3 commits in PR #1361 lint converted to ES6 according to the plan in #1561 convert scatter canvas examples to D3v5

gordonwoodhull · 2019-09-19T19:24:18Z

Hi @kum-deepak, I have merged everything to the es6 branch, including the changes to examples and the canvas scatter plot.

Now this PR is really merged!!

kum-deepak added the ES6 label Aug 18, 2019

gordonwoodhull closed this Sep 9, 2019

gordonwoodhull reopened this Sep 9, 2019

kum-deepak force-pushed the charts branch from 3718d72 to 0ac4b6b Compare September 9, 2019 15:22

gordonwoodhull closed this Sep 19, 2019

gordonwoodhull reopened this Sep 19, 2019

gordonwoodhull reviewed Sep 19, 2019

View reviewed changes

gordonwoodhull merged commit 2b70aeb into dc-js:es6 Sep 19, 2019

gordonwoodhull force-pushed the es6 branch from 33112e2 to 91ee43c Compare September 19, 2019 18:24

kum-deepak deleted the charts branch September 28, 2019 06:13

Charts to ES6 classes #1561

Charts to ES6 classes #1561

Conversation

kum-deepak commented Aug 18, 2019

kum-deepak commented Aug 18, 2019

gordonwoodhull commented Aug 18, 2019

kum-deepak commented Aug 18, 2019

gordonwoodhull commented Aug 19, 2019

gordonwoodhull commented Aug 19, 2019 • edited Loading

kum-deepak commented Aug 20, 2019

gordonwoodhull commented Aug 20, 2019

kum-deepak commented Aug 20, 2019

kum-deepak commented Aug 20, 2019

gordonwoodhull commented Aug 20, 2019

gordonwoodhull commented Aug 20, 2019

gordonwoodhull commented Aug 20, 2019

kum-deepak commented Aug 20, 2019

gordonwoodhull commented Aug 20, 2019 • edited Loading

kum-deepak commented Aug 22, 2019

gordonwoodhull commented Aug 24, 2019

gordonwoodhull commented Aug 25, 2019

kum-deepak commented Aug 25, 2019

kum-deepak commented Aug 25, 2019

gordonwoodhull commented Aug 25, 2019

gordonwoodhull commented Aug 26, 2019

"private methods" of classes

_tweenPie

_pvt_onClick

kum-deepak commented Aug 26, 2019

gordonwoodhull commented Aug 27, 2019

gordonwoodhull commented Sep 3, 2019

kum-deepak commented Sep 9, 2019

gordonwoodhull commented Sep 9, 2019

kum-deepak commented Sep 9, 2019

gordonwoodhull commented Sep 9, 2019 • edited Loading

kum-deepak commented Sep 9, 2019

kum-deepak commented Sep 9, 2019

gordonwoodhull commented Sep 10, 2019

gordonwoodhull commented Sep 11, 2019

kum-deepak commented Sep 11, 2019

gordonwoodhull commented Sep 11, 2019

kum-deepak commented Sep 11, 2019

kum-deepak commented Sep 14, 2019

gordonwoodhull commented Sep 16, 2019

kum-deepak commented Sep 16, 2019

gordonwoodhull commented Sep 16, 2019

kum-deepak commented Sep 19, 2019

gordonwoodhull commented Sep 19, 2019 • edited Loading

gordonwoodhull commented Sep 19, 2019

gordonwoodhull commented Sep 19, 2019

gordonwoodhull Sep 19, 2019

Choose a reason for hiding this comment

kum-deepak Sep 19, 2019

Choose a reason for hiding this comment

kum-deepak commented Sep 19, 2019

kum-deepak commented Sep 19, 2019

gordonwoodhull commented Sep 19, 2019

kum-deepak commented Sep 19, 2019

gordonwoodhull commented Sep 19, 2019

gordonwoodhull commented Sep 19, 2019

gordonwoodhull commented Aug 19, 2019 •

edited

Loading

gordonwoodhull commented Aug 20, 2019 •

edited

Loading

gordonwoodhull commented Sep 9, 2019 •

edited

Loading

gordonwoodhull commented Sep 19, 2019 •

edited

Loading