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 circular dependencies #2389

Merged
merged 5 commits into from Jun 23, 2018

Conversation

Projects
None yet
3 participants
@amenadiel
Copy link
Contributor

amenadiel commented Jun 11, 2018

Curently, building with rollup using npm run build:js shows two warnings in the output

(!) Circular dependency: src/core.js -> src/axis.js -> src/core.js
(!) Circular dependency: src/core.js -> src/axis.js -> src/util.js -> src/core.js

I've refactored the code in the following way:

  • src/util.js contains the root c3 object, as well as modules Chart and Component so it doesn't need to import from src/core
  • src/chart.js imports everything it needs from ./class and ./util so it doesn't import from ./core thus removing the circular dependency
  • src/core.js imports also c3, Chart and Component from src/util.js
  • Added src/chart-internal.js which in turn imports d3. Every other module doesn't use d3 directly so this is the only component that should access d3.
  • I've changed the way in which d3 is accessed, since using it as
$$.d3 = window.d3 ? window.d3 : typeof require !== 'undefined' ? require("d3") : undefined;  

isn't the proper way to deal with external modules in the ES6 approach. Instead, it should be imported and listed in rollup.config.js as global/external

  • Also, in rollup.config.js I added the file property to the output object, because it is the reccomended way to point to the generated file, so you can just call rollup -c instead of doing rollup -c > c3.js (this approach also allows to output to different files, such as an UMD build along an ES6 build, if you ever wanted to do so). Besides, this results in a friendlier build output avoiding to have the whole source code piped to STDOUT. The package.json has been modified accordingly.

PD

The resulting UMD module (not included in the commit as per the PR directives) will either use global d3 or require it as:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('d3')) :
    typeof define === 'function' && define.amd ? define(['d3'], factory) :
    (global.c3 = factory(global.d3));
}(this, (function (d3) {
   ...
})));

Which accomplishes the same purpose of the old approach

$$.d3 = window.d3 ? window.d3 : typeof require !== 'undefined' ? require("d3") : undefined;  

This also allows an explicit AMD dependency using define instead of require, although requirejs and SystemJS should support using CommonJS requires as rell.

@amenadiel amenadiel force-pushed the HuasoFoundries:refactor_circular_deps branch 2 times, most recently from 1d26672 to ad63924 Jun 11, 2018

Refactors circular dependencies
 - Adds src/chart-internal.js
 - doesn't need to pass c3.js as argument to rollup
 - don't commit c3.js nor c3.min.js
 - formatting rollup.config.js
 - keep util.js format
 - remove unused imports from axis, core and util

@amenadiel amenadiel force-pushed the HuasoFoundries:refactor_circular_deps branch from 8e9cf64 to 0b7bf8d Jun 11, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 11, 2018

Codecov Report

Merging #2389 into master will increase coverage by 0.81%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2389      +/-   ##
==========================================
+ Coverage   77.23%   78.05%   +0.81%     
==========================================
  Files          51       52       +1     
  Lines        4138     4137       -1     
==========================================
+ Hits         3196     3229      +33     
+ Misses        942      908      -34
Impacted Files Coverage Δ
src/axis.js 96.08% <ø> (ø) ⬆️
src/core.js 88.78% <ø> (-0.46%) ⬇️
src/chart-internal.js 100% <100%> (ø)
src/util.js 92.15% <100%> (+14.37%) ⬆️
src/api.zoom.js 75.67% <0%> (+21.62%) ⬆️
src/api.axis.js 100% <0%> (+68.75%) ⬆️

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 b30eeee...1521d6a. Read the comment docs.

@@ -31,7 +68,7 @@ export var isEmpty = function (o) {
return typeof o === 'undefined' || o === null || (isString(o) && o.length === 0) || (typeof o === 'object' && Object.keys(o).length === 0);
};
export var notEmpty = function (o) {
return !c3_chart_internal_fn.isEmpty(o);
return !c3.chart.internal.fn.isEmpty(o);

This comment has been minimized.

@amenadiel

amenadiel Jun 11, 2018

Author Contributor

You already have c3.chart.internal.fn defined in this module, so you don't need to import it's alias from ./core

@@ -13,7 +50,7 @@ export var isString = function (o) {
return typeof o === 'string';
};
export var isUndefined = function (v) {
return typeof v === 'undefined';
return typeof v === 'undefined';

This comment has been minimized.

@amenadiel

amenadiel Jun 11, 2018

Author Contributor

This line was indented incorrectly

@@ -1,6 +1,5 @@
import CLASS from './class';
import { Component } from './core';
import { isValue, isFunction, isString, isEmpty } from './util';
import {isValue, isFunction, isString, isEmpty, Component } from './util';

This comment has been minimized.

@amenadiel

amenadiel Jun 11, 2018

Author Contributor

Component is exported by ./util so you don't need to import it from ./core anymore

@@ -9,7 +9,7 @@
"docs": "bundle exec middleman",
"build": "npm run build:js && npm run build:css",
"build:js": "npm run build:js:rollup && npm run build:js:uglify",
"build:js:rollup": "rollup -c > c3.js",
"build:js:rollup": "rollup -c",

This comment has been minimized.

@amenadiel

amenadiel Jun 11, 2018

Author Contributor

target output of rollup is configured on rollup.config.js so you don't need to pipe STDOUT to a file explicitly

banner: `/* @license C3.js v${pkg.version} | (c) C3 Team and other contributors | http://c3js.org/ */`
banner: `/* @license C3.js v${pkg.version} | (c) C3 Team and other contributors | http://c3js.org/ */`,
globals:{
d3:'d3'

This comment has been minimized.

@amenadiel

amenadiel Jun 11, 2018

Author Contributor

This will list d3 as a dependency in the top of the UMD build, instead of requiring d3 from ChartInternal

This comment has been minimized.

@amenadiel

amenadiel Jun 22, 2018

Author Contributor

this one does no longer apply. D3 is required from inside ChartInternal as always

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jun 21, 2018

Thanks for the contribution! I agree with most changes.

But I'd like to keep the timing of requiring d3 unchanged. Some users seem loading c3 and d3 in parallel, and this change breaks that situation. (We did the same change before and that caused the problem. See #2060 ) So I'd prefer keep it requiring inside ChartInternal constructor.

Other part seems good to me 👍

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jun 22, 2018

I see. So the edge case scenario would be if someone loaded both d3 and c3 using async defer

But in that case, if you had no control over which script loaded first, you wouldn't either know when C3 has finished loading, and therefore you couldn't instance a new chart, unless you rely on setting a timeout.

I don't know... keeping a require among ES6 modules seems suboptimal. I can rollback that specific part for this PR to be merged, anyway

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jun 22, 2018

you wouldn't either know when C3 has finished loading, and therefore you couldn't instance a new chart, unless you rely on setting a timeout.

You are right. I guess probably they instanciate the chart after DOMContentLoaded or $(document).ready() (I think that's a common manner in frontend dev).

I don't know... keeping a require among ES6 modules seems suboptimal. I can rollback that specific part for this PR to be merged, anyway

I would appreciate if you rollback that part 🙏

@kt3k

kt3k approved these changes Jun 22, 2018

Copy link
Member

kt3k left a comment

LGTM!

Thanks for updating!

Thanks for test additions as well!

@amenadiel

This comment has been minimized.

Copy link
Contributor Author

amenadiel commented Jun 22, 2018

Okay, I restored the way d3 is required to

$$.d3 = window.d3 ? window.d3 : typeof require !== 'undefined' ? require("d3") : undefined;  

However, regarding the inclusion of d3, I'll be honest:

I was working in a branch that bundled c3 along d3. If you just import d3, the weight of the unminified build was roughly 900 kb. (which is the sum of the unminified builds of c3 and d3, give or take). BUT, if you only import the D3 components you really use:

export var d3 = {
     arc,
     area,
     brushSelection: brushSelection,
     brushX: brushX,
     brushY: brushY,
     csv: csv,
     curveBasis,
     curveBasisClosed,
     curveBasisOpen,
     curveBundle,
     curveCardinal,
     curveCardinalClosed,
     curveCardinalOpen,
     curveLinear,
     curveLinearClosed,
     curveMonotoneX,
     curveStep,
     curveStepAfter,
     curveStepBefore,
     customEvent: customEvent,
     drag: drag,
     easeLinear: easeLinear,
     extent: extent,
     get event () { return event; },
     get format () { return format; },
     get formatPrefix () { return formatPrefix; },
     get timeFormat () { return timeFormat; },
     get timeParse () { return timeParse; },
     get utcFormat () { return utcFormat; },
     get utcParse () { return utcParse; },
     interpolate: interpolate,
     json: json,
     line,
     max: max,
     merge: merge,
     min: min,
     mouse: mouse,
     pie,
     rgb: rgb,
     scaleLinear: scaleLinear,
     scaleTime: scaleTime,
     schemeCategory10: schemeCategory10,
     select: select,
     selectAll: selectAll,
     selection: selection,
     selector: selector,
     selectorAll: selectorAll,
     set: set,
     timeSecond: timeSecond,
     transition: transition,
     tsv: tsv,
     zoom: zoom,
     zoomIdentity: zoomIdentity,

 };

The bundle size is ~ 650 kb, so you shave off 250kb from d3. It could be useful if someone doesn't want to require d3 via a script tag. You could have extra c3.bundle.js and c3.bundle.min.js scripts available for that particular use case.

TL/DR

Optional builds that bundle d3, using only required d3 imports. Can be built along regular c3.js by referencing different chart-internal.js scripts.

  • Unminified size: 650KB vs 394KB
  • Minified size: 306KB vs 187KB

You can see those changes at HuasoFoundries@0f3e4d0

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jun 23, 2018

Hmm..smaller bundle looks interesting. It's good for lower bandwidth environment and possibly some users might be interested using it...
But, on the other hand, I feel slimmed version of d3 is a bit hard for us to keep maintaining in the future...

Anyway we take this refactoring first. Thanks for the contributions!

@kt3k kt3k merged commit 22b06bb into c3js:master Jun 23, 2018

2 checks passed

ci/circleci: test Your tests passed on CircleCI!
Details
codecov/project 78.05% (+0.81%) compared to b30eeee
Details

@amenadiel amenadiel deleted the HuasoFoundries:refactor_circular_deps branch Jul 6, 2018

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