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

Convert project to ES6 with babel, webpack & eslint. #1175

Closed
wants to merge 1 commit into from

Conversation

mtraynham
Copy link
Contributor

@mtraynham mtraynham commented Jul 6, 2016

This is a re-implementation of #937 (which I'm going to close). So much of the code changed with the docs, that a rebase was almost too complicated to perform and I messed things up along the way...

I know @gordonwoodhull, you were a bit hesitant in the past, but I'll rebase this pretty often to keep it up to date in case we ever decide to migrate.

I should note that this is a starter for what could be. Webpack has gotten incredibly expressive, and a modular/component layout is much more beneficial in the long term. A few things I'd like to point out, we could potentially:

  • Have a css (or less/scss) sheet for each chart type which is imported by the chart class itself (i.e. import './bar-chart.scss'. This would potentially allow users to download only parts of the library and have it all still working. Angular-Material is a good example of how this works.
  • The above would also reduce the size of code for end users. If only a bar chart is required, there should be no need to download 500 Kb of a library. Shipping the library as modules would allow end users through npm to select only the modules they need and bundle them as necessary.
  • Supporting both d3 v3 & v4. Webpack has ways to rewrite access to global variables (such as d3). We could potentially ship a dc for v3 and v4 (related to Support D3 v4 #1173), by simply converting the import * as d3 from 'd3' to the v4 separate projects, and then have webpack rewrite this using an associative array mapping back to v3. We would likely need separate branches/bower/package to support both releases, but the code would always be the same.

Other notes:

  • I've updated travis to use Node 5 & 6, as 0.10 is EOL. Node 4 could be supported, but it doesn't relax peer dependencies and grunt-eslint (depends on eslint 3) has some conflicts with the very well thought out eslint-config-airbnb and it's dependencies.

Known issues:

Breaking changes:
dc.disableTransition, dc.enableDebugLog and dc.dateFormat are now getter/setters.

@mtraynham mtraynham mentioned this pull request Jul 6, 2016
5 tasks
@mtraynham mtraynham force-pushed the es6 branch 5 times, most recently from 3206e76 to fdfd734 Compare July 6, 2016 22:19
@gordonwoodhull
Copy link
Contributor

Thanks @mtraynham!

Porting to d3.js v4 is not just a name substitution. There are breaking changes on most components (notably d3.selection). It doesn't sound like it will be so difficult but it will touch most of the code.

Are Webpack import statements compatible with Rollup import statements? I know they are both ES6 but are they the same ES6? Honest question. I ask because upgrading to d3.js v4 (#1173) is a higher priority for me, and we could switch to Rollup as part of that effort.

It looks like Rollup is somewhat more powerful than Webpack, in that it can include only the needed code from a module. I don't know how much this affects dc.js, given that it consists mostly of big class-like functions.

Sorry I don't plan to upgrade to ES6 soon. The cost of transpilation on debugging concerns me too much. And fixing bugs is more important to me than using the latest tools.

But thanks for doing this. People can use your fork if they want an ES6 dc.js!

@mtraynham
Copy link
Contributor Author

mtraynham commented Jul 10, 2016

Just going to address some of your points 😄

Porting to d3.js v4 is not just a name substitution

Can you point me to the breaking changes? I saw the change log, but must have missed the breaking changes section. I thought it was mostly package separation of concerns and improvements to the API.
To clarify, my intention here is:

import {select, selectAll} from 'd3-selection'; // v4
import {select, selectAll} from 'd3'; // v3

You could write the v4 version and we could have Webpack translate d3-selection to d3 if necessary for v3.

in that it can include only the needed code from a module.

Webpack is the same as they both support the CommonJS require spec; it's just slightly different because Rollup has ES6 imports builtin, whereas Webpack uses babel as a preprocessor to get all the benefits of ES6. You can import extent from 'd3/src/extent' just the same and it will only include that in your bundled build. If you don't import a module, Webpack won't include it. Webpack 2.0 will include tree-shaking which will drop unused modules, further reducing the size of the bundle. I would tend to believe Webpack is a bit more battle-tested, with community size, number of contributors and feature set, but the two are likely interchangeable.

Webpack/Rollup is a bit beside the point. People should be able to import charts directly (e.g. import barChart from 'dcjs/src/bar-chart') when installing from npm and use Webpack/Browserify/Rollup, or whichever module bundler of their own choosing. Webpack here is simply to provide a build for bower that mimics what we currently have, a browser global library. It looks as if d3 v4 doesn't even support bower directly.

Sorry I don't plan to upgrade to ES6 soon.

In all fairness, Rollup is an ES6 bundler and d3 v4 is entirely dependent on the ES6 spec; so code created by Rollup is going to be transpiled to a bundle in a similar fashion. This PR only contains two other things from ES6 which Rollup can't handle, const/let and arrow functions, both of which are trivial to decypher once converted back to ES5.

For a further comparison on Webpack vs. Rollup, the author of Rollup has a long comment here. To kind of refute one of his points, where he says Webpack is not a great library bundler, it's really no different. You have an index.js file which just exports (or re-exports) everything, just like the d3-selection index file I posted above and just like this PR.

I know you have hesitations, so as I said, I'll just rebase every now and then.

@gordonwoodhull
Copy link
Contributor

I haven't read everything about v4 but there are numerous mentions of breaking changes. That's what happens when you make improvements to the API, and that's the way it should be.

@ruhley might be able to elaborate because he started trying to do the port on his fork.

I understand that you've mostly done a mechanical translation which only introduces changes which are easy to debug after transpilation. But once we open that door then stuff will enter which is more difficult. Like destructuring assignment and so on. So why not go the route where we really are just using ES5 with modules, like d3 did? I am pretty sure that is the extent of their ES6 usage, but please correct me if I'm wrong.

I do encourage you to continue your fork and everyone who wants to use it, should. I'll keep this PR open as a pointer to your repo, but I do not intent to merge this until ES6 reaches the browsers.

@ruhley
Copy link
Contributor

ruhley commented Jul 10, 2016

I had a quick attempt at converting to d3 v4 and you can see my results at #1173 (comment). You can see in my comment that some things are just a rename and seem to work without any other changes, but some things have completely changed. So I think support d3 v3 and v4 with the same library is a bad idea. It could start out simple, but would most likely end up in a big mess.

In saying that I love the rest of the stuff you have done and I am in full support of using ES6. Your changes don't seem to change any logic and if the tests still pass then I would be happy for the upgrade.

@mtraynham
Copy link
Contributor Author

mtraynham commented Jul 10, 2016

@ruhley Good to know and thanks 😊 ! I wonder if it's mostly the layout stuff that's incompatible... I noticed that d3.layout.pie was also renamed to d3.pie and exists in the d3-shape package. I can't seem to find breaking changes documentation anywhere though...

I made this table as a v4 migration reference, so maybe you will find it useful.

@kum-deepak
Copy link
Collaborator

Great idea!

@ialarmedalien
Copy link

I've been using this PR as a base for some other code -- it's great to be able to work with dc.js in ES6; thanks for the work you've done on it! I noticed a tiny discrepancy in utils.js, looks like a weird copy/paste artefact around line 272:

let _iunter = 0;
utils.uniqueId = function () {
    return ++_iunter;
};

The variable _iunter is _idCounter in the master branch. Not important, but it just looks a bit odd!

@mtraynham
Copy link
Contributor Author

Thanks @ialarmedalien! Probably occurred during a regex removal of the dc global throughout the code. I'll rebase with it fixed.

@mtraynham mtraynham force-pushed the es6 branch 2 times, most recently from 5e426bf to efbcb67 Compare October 20, 2017 02:21
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 4, 2018

Sorry, I am closing this to be clear dc-js/dc.js is not going this way. We're following d3 and using what's supported by the browsers, and I hope to move to import/export and rollup and modularize the charts in the same way that d3 is a lot of smaller, independent modules now.

I know, everyone loves transpilation and using the most modern everything, but it really causes a lot of trouble when trying to debug code on any developer tools !== Chrome. Source maps are really tricky and apparently no one outside of Google cares about them.

I encourage anyone to use @mtraynham's fork if they want a transpiled dc.js

@mtraynham
Copy link
Contributor Author

@gordonwoodhull Thanks for the feedback.

I'm not sure I completely follow the reasoning. Rollup transforms the source in a manner that is not 1 to 1 with the originally source code, instead providing a bundle. I concede that d3 doesn't use any other features of ES6 and the bundle is likely more readable to a developer, but it does use the ES6 import/export syntax and modules to work with Rollup instead of just concatenating everything.

So my question is, are you just concerned about the usage of const/let & arrow functions which are transformed by babel and the usage of Webpack instead of Rollup?

If that's the case, it would still be nice to ship the source as ES5 with ES6 import/export syntax (which has been done as part of this PR), so users can benefit from module loaders that tree-shake unused code. D3 does this, and if you npm install d3-array, you can see an example.

The install has both a build (the bundle) and src (the original source).
1

Within the package.json file it has references to main, module and jsnext:main, which tells module bundlers where they can pull source code from.
2

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 4, 2018

Yes, that is what I meant, import/export rather than require. (I've corrected above.)

What you're describing is exactly what I mean by modularization of the build. I want to follow what d3v4 did.

The big win is that when someone makes their own version of a chart, people can use it immediately instead of waiting for it to be merged into dc-js. This is especially important when people make radically different versions of charts which break tests and previous functionality. Potential users of new features get very impatient with me for not merging stuff I can't safely merge without breaking 100s of other applications. They could use the fork instead.

Also, there will be no real difference between charts contributed to the dc-js organization and charts hosted elsewhere.

The downside is that there will be >24 tiny libraries with one class each. I sort of understand how import/export work but I haven't yet learned how Mike deals with versioning and dependencies. He has said that it's pretty tedious to update patterns across repos.

@ialarmedalien
Copy link

ialarmedalien commented Apr 5, 2018

I started some d3-esque modularisation of the base mixin of this fork of the codebase -- split it into 16 smaller modules that get gathered up to create base-mixin.js. It was not possible to completely eliminate crossover/dependency between different modules and there are some places where there are dependencies on other dc.js modules (e.g. core.js). The motive was to integrate/reuse dc.js with some other d3-related libraries that I use and like (particularly d3.chart and kotojs, which I think handles the enter/update/exit cycle nicely, and which splits charts into individually-specified reusable layers), but it ended up being too much hassle.

@gordonwoodhull
Copy link
Contributor

Hi @ialarmedalien, that's an interesting data point, thanks!

Yes, I think it would be very difficult to split dc.js "horizontally" as all the features interact combinatorially. My thought is just to put each class or base/mixin in its own module, so that people can import only what they want. It would mostly follow the class hierarchy with a few other modules like core.

No guarantee that this would achieve its goals, and it's a lot of work just to find out.

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

Successfully merging this pull request may close these issues.

5 participants