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

Refactors classes + bugfix + improves coverage #2405

Merged
merged 8 commits into from Jul 14, 2018

Conversation

Projects
None yet
4 participants
@amenadiel
Copy link
Contributor

amenadiel commented Jul 6, 2018

Uses classes prototype instead of their aliases

  • Class Chart was moved to its own script.
  • Class AxisInternal was moved to its own script. It's only needed by Class Axis
  • Definition of Chart removed from core.js
  • Definition of AxisInternal removed from axis.js
  • Alias c3_chart_fn replaced by Chart.prototype where is due
  • Alias c3_chart_internal_fn replaced by ChartInternal.prototype where is due
  • spec/data.convert.js renamed to spec/data.convert-spec.js for consistency
  • karma loads patterns spec/*-helper.js and spec/*-spec.js

Fixes Chart.prototype.legend.hide behavior

  • It was calling $$.updateAndRedraw({withLegend: true});
  • It should call $$.updateAndRedraw({withLegend: false});

Also, added some tests to improve coverage

amenadiel added some commits Jul 6, 2018

Uses classes prototype instead of their aliases
 - Class chart was moved to its own script.
 - Class AxisInternal was moved to its own script. It's only needed by Class Axis
 - Definition of Chart removed from core.js
 - Definition of AxisInternal removed from axis.js
Replaces aliases of Class prototypes
  - Alias c3_chart_fn replaced by Chart.prototype where is due
  - Alias c3_chart_internal_fn replaced by ChartInternal.prototype where is due
  - spec/data.convets.js renamed to spec/data.convert-spec.js for clarity
Fixes Chart.prototype.legend.hide behavior
 - It was calling $$.updateAndRedraw({withLegend: true});
 - It should call $$.updateAndRedraw({withLegend: false});
adds tests to improve coverage
restores c3.js and c3.min.js to master version

fixes linter error

@amenadiel amenadiel force-pushed the HuasoFoundries:refactors_classes branch from 6a685ac to 15837d8 Jul 6, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 6, 2018

Codecov Report

Merging #2405 into master will increase coverage by 2.61%.
The diff coverage is 96.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2405      +/-   ##
==========================================
+ Coverage   78.14%   80.75%   +2.61%     
==========================================
  Files          52       54       +2     
  Lines        4136     4246     +110     
==========================================
+ Hits         3232     3429     +197     
+ Misses        904      817      -87
Impacted Files Coverage Δ
src/api.data.js 100% <100%> (ø) ⬆️
src/clip.js 100% <100%> (ø) ⬆️
src/category.js 100% <100%> (ø) ⬆️
src/scale.js 100% <100%> (ø) ⬆️
src/api.category.js 16.66% <100%> (ø) ⬆️
src/axis.js 97.55% <100%> (+1.46%) ⬆️
src/domain.js 93.67% <100%> (ø) ⬆️
src/api.x.js 100% <100%> (+83.33%) ⬆️
src/shape.js 78.68% <100%> (ø) ⬆️
src/text.js 98.68% <100%> (ø) ⬆️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b11a28...42327e6. Read the comment docs.

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 7, 2018

Thank you for the great work! @amenadiel

I like this change of the way of referencing the prototypes of classes. This seems more natural to my eyes.

And thanks for adding test cases!

@kt3k

kt3k approved these changes Jul 7, 2018

@kt3k kt3k requested a review from panthony Jul 7, 2018

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 8, 2018

I'm going to wait for other members' opinion for a while. If there is no other opinion, then I'll merge and release this.

@panthony

This comment has been minimized.

Copy link
Contributor

panthony commented Jul 8, 2018

@kt3k I think this is a first step in order to properly separate the logic of each components of the chart and ease the refactoring of draw logic (where we re-create/resize several times the same thing on a single draw).

I took a similar path (here and there) in my fork.

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jul 10, 2018

@kt3k I think this is a first step in order to properly separate the logic of each components of the chart and ease the refactoring of draw logic (where we re-create/resize several times the same thing on a single draw).

I took a similar path (here and there) in my fork.

@panthony it looks great. The main class has so many methods attached to the prototype that is pretty cumbersome to track what goes where, which in turn makes unit testing more difficult. I mean, to reach certain parts of the "private" methods switch of if...else statements, you must go as far as declaring new charts over and over, because said methods are called from elsewhere and tied to the initial options.

src/util.js Outdated
@@ -1,36 +1,18 @@
import {ChartInternal} from './chart-internal';
import {Chart} from './chart';

This comment has been minimized.

@panthony

panthony Jul 10, 2018

Contributor

Not sure if this is the scope of this PR but in my opinion the util.js file should NOT import chart/chart internal.

This file should be as "low level" at it can be.

Otherwise we are bound to have circular dependency when importing this file

This comment has been minimized.

@amenadiel

amenadiel Jul 10, 2018

Author Contributor

You mean util.js should be low level in the sense of being just a collection of functions? In that case, c3 could be elsewhere.

However, util.js exports Component which in turn is used by Axis to extend ChartInternal and c3.chart.internal. This means Component needs the definition of ChartInternal and c3 (of course) and you can't define c3 without importing Chart and ChartInternal.

However, I'm confused, because Axis is instanced as

$$.axis = new Axis($$)

(where $$ is the instance of ChartInternal).

And when instancing Axis, the side effect are:

1.- Axis (instance).owner = ChartInternal instance

2.- c3 is changed from:

var c3 = {
    version: "0.6.3",
    chart: {
        fn: Chart.prototype,
        internal: {
            fn: ChartInternal.prototype,
        }
    },
    generate: function(config) {
        return new Chart(config);
    }
};

to

var c3 = {
    version: "0.6.3",
    chart: {
        fn: Chart.prototype,
        internal: {
            fn: ChartInternal.prototype,
            axis: {
                fn: Axis.prototype,
                internal: {
                    fn: AxisInternal.prototype
                }
            }
        }
    },
    generate: function(config) {
        return new Chart(config);
    }
};

the last one could be handled if we moved the definition of c3 to a separate file which imported Chart, ChartInternal, Axis and AxisInternal.

whereas the first one could be handled simply by defining Axis as

export default class Axis {
    constructor(owner) {
        this.owner = owner;
        this.d3 = owner.d3;
        this.internal = AxisInternal;
    }
}

util.js also exports functions isEmpty and notEmpty where the second one is defined as

export var notEmpty = function (o) {
    return !ChartInternal.prototype.isEmpty(o);
};

again, this would mean that util.js needs ChartInternal. BUT, it would be as simple as declaring notEmpty as

export var notEmpty = function (o) {
    return !isEmpty(o);
};

Because it is by all means a static function, even if they are later attached to ChartInternal.prototype.

I'll make some arrangements to see if, with some refactoring, this could pass the tests keeping util.js just a bunch of static functions.

This comment has been minimized.

@panthony

panthony Jul 10, 2018

Contributor

Sorry for such a short response to such a huge reply but:

You mean util.js should be low level in the sense of being just a collection of functions? In that case, c3 could be elsewhere.

That's exactly what I mean !

We shouldn't have to depend on the whole "ChartInternal" class for such small functions.

However, util.js exports Component which in turn is used by Axis to extend ChartInternal and c3.chart.internal. This means Component needs the definition of ChartInternal and c3 (of course) and you can't define c3 without importing Chart and ChartInternal.

And that is why :)

Because it is by all means a static function, even if they are later attached to

I think they shouldn't even be attached to the prototype... They are just static utility functions that you import when needed.

@panthony

This comment has been minimized.

Copy link
Contributor

panthony commented Jul 10, 2018

@kt3k Merging this PR should require ideally a major version bump (but I guess in c3js case a minor) as any workaround/fix people did overriding an internal method won't work anymore.

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jul 10, 2018

@panthony I refactored a bit, based in your review.

  • util.js exports just static functions (no imports).
  • core.js defines c3 with its axis and axisinternal properties
  • Component function is removed
  • Axis now does just what's intended: sets ChartInternal as its owner.
  • Added test for api.x.js

@amenadiel amenadiel force-pushed the HuasoFoundries:refactors_classes branch from bdddf40 to 886ac3a Jul 10, 2018

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jul 10, 2018

  • Added test for drag behavior
@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 11, 2018

@panthony

@kt3k Merging this PR should require ideally a major version bump (but I guess in c3js case a minor) as any workaround/fix people did overriding an internal method won't work anymore.

Thanks for pointing it. I didn't intend to do breaking change in this PR.

Now I think the overriding functionality is being kept in core.js ( https://github.com/c3js/c3/pull/2405/files#diff-7eb52b366866677666470e019283c8eaR24 )
and we don't need to bump minor (breaking) version. How do you think?

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jul 11, 2018

At least the extensions I,ve seen attach themselves to c3.iternal.fn or c3.chart.internal.fn

Regarding utils.js since it's a bunch of functions, if we don't attach it to the prototype there's no way to unit test.

@panthony

This comment has been minimized.

Copy link
Contributor

panthony commented Jul 11, 2018

@amenadiel Thanks for this huge work, If I were nitpicky I'd say it would have been better without all the changes related to formatting.

It's harder to see if there is something actually meaningful that were updated.

Regarding utils.js since it's a bunch of functions, if we don't attach it to the prototype there's no way to unit test.

Not seeing why ? Create a -spec.js file, import the util function and call it? Not sure I'm following.

@kt3k For the breaking change maybe I was wrong I though people where doing c3_chart_internal_n.XXXX but it may have been c3.chart.internal.fn. which shouldn't change with this PR.

@amenadiel amenadiel force-pushed the HuasoFoundries:refactors_classes branch from ccd66b5 to 700f266 Jul 11, 2018

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jul 11, 2018

@amenadiel Thanks for this huge work, If I were nitpicky I'd say it would have been better without all the changes related to formatting.

Hmm, I ran jsbeautify to restore occurences of function() to function () but it came along other reformattings, such as breaking multiple var declarations into one line for each variable (which is a good practice but changes format anyway).

Not seeing why ? Create a -spec.js file, import the util function and call it? Not sure I'm following.

Silly me, I was half asleep. Yeah, I forgot we could use ES6 imports in the specs. I'm used to run karma-jasmine on AMD builds while you're browserifying beforehand. So okay, I added util-spec.js to test everything but getPathBox which is tested on share.bar-spec.js for I need a chart with a bar classed shape to test it.

@amenadiel amenadiel force-pushed the HuasoFoundries:refactors_classes branch from 700f266 to a96293c Jul 11, 2018

@amenadiel amenadiel force-pushed the HuasoFoundries:refactors_classes branch from 1103892 to 07e4dee Jul 11, 2018

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 14, 2018

@amenadiel
Thanks again for adding a lot of tests!

I think there is no issue remaining. I'm going to merge and release this.

@kt3k kt3k merged commit 9556e38 into c3js:master Jul 14, 2018

1 check passed

ci/circleci: test Your tests passed on CircleCI!
Details
@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 14, 2018

Released as 0.6.5.

@amenadiel amenadiel deleted the HuasoFoundries:refactors_classes branch Jul 25, 2018

@kt3k kt3k referenced this pull request Nov 1, 2018

Merged

fix regions.remove #2517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment