Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

dc.js 4.0: d3 6.0 and ES6 #1520

Closed
gordonwoodhull opened this issue Mar 13, 2019 · 25 comments
Closed

dc.js 4.0: d3 6.0 and ES6 #1520

gordonwoodhull opened this issue Mar 13, 2019 · 25 comments
Labels

Comments

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Mar 13, 2019

There has been a lot of interest in the dc.js community for an ES6 version, but it was not a priority to me as long as we had to transpile and run on IE. (*)

However, d3 6.0 will finally drop IE - see d3/d3-array#87. Glory day! We can use ES6 natively!

I'd be happy to work with someone or a team of people who want to port dc.js to Rollup, NO BABEL or transpilation, and ES6 modules. I think we should keep the code in one repo but have lots of modules in it. ES6 modules should allow anyone to mix and match their own dc.js charts or versions of dc.js charts with those in the official repo, and only take what they need.

I don't have the time to do all this work myself, but I have the time to do code review, make decisions and commit PRs for this project.

Challenges:

  1. The dependency between modules is complicated. I think we will save ongoing effort by staying in one repo with one version number, but we still need to figure out the dependencies beyond the class hierarchy. I'll bet there are many places where the dependencies are not pure, yet we want tree-shaking to work, etc.
  2. Currently dc.js does not use prototype inheritance, so the path to ES6 classes is not completely clear. I doubt it will lead to significant performance improvements, but it would be nice to use idiomatic ES6 classes. There may be things we are doing, like adding and changing methods on other classes, which will not work or will cause subtle bugs.
  3. As much as possible, we should be able to keep code written for DCv3 working with v4, but where ES6 helps the dc.js API, we should use it. ES6 classes may break the interface a little. I don't know if we'll have trouble with the places where people replace methods like yAxisMin in order to get the right effect.

We can't fix everything at once - let's try to leave drastic interface changes for later, wherever possible.

I bet a lot of people out there have already gone through the ES5->ES6 transformation and know the best practices. Anyone have time to take this up? Anyone have experience with Rollup the way it is used to bundle D3?

Please leave a comment if you're interested in working on this: doing the translation, review PRs, help debug, or just offer advice.

I know some people worked with @mtrayham's fork #1175. I closed the PR because I didn't want to transpile & still don't want to. I also want to use rollup instead of webpack because it is simpler. Nonetheless it's the right direction - did anyone try to merge that fork with the D3v4/5 changes in 3.x?

I don't know how many d3 modules have dropped ES5 and moved to ES6. I know of d3-array and d3-force so far, but d3 6.0 has not been released yet. We have the opportunity to get ahead of the curve and release DCv4 soon after D3v6.

(*) I continue to be opposed to transpilation, since we frequently need to debug incompatibilities using the devtools of every browser. In my experience, only the Chrome debugger works reliably for debugging transpiled code. However, if someone else wants to maintain the toolchain for transpiling dc.js 4.0, and debug any issues that arise, I am cool with that!

@gordonwoodhull
Copy link
Contributor Author

Alright, no volunteers! Not even a reactji. 🙁

Anyone think this is a bad idea? Confusing? TL;DR?

@gordonwoodhull
Copy link
Contributor Author

Thanks! 👍 I'll keep looking into how to do this. If anyone with Rollup experience wants to jump in and get started, feel free!

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Jul 30, 2019

D3 6.0 is in progress: d3/d3#3333

@kum-deepak
Copy link
Collaborator

It will be my first ES6 work, but I will like to take a shot 😃

Wish me luck!

@gordonwoodhull
Copy link
Contributor Author

That's fantastic, thanks @kum-deepak!

I have started using ES6 in another project and I am really enjoying it. It won't fix any dc.js bugs but it will make the code a lot nicer, especially modules.

Please reach out with any questions or concerns.

@kum-deepak
Copy link
Collaborator

Work is getting carried out in PR #1540. Progress so far:

  • rollup setup as a grunt task
  • Code converted to ES6 modules - it uses import/export
  • Minimum refactoring
    - renaming local variable that were shadowing imported stuff
    - dc.constants was causing circular dependency, so extracted out
  • uglify fails, commented out as of now
  • other than 4 all test cases pass
  • all examples seem to work

Stage set for deeper work 😃

@gordonwoodhull
Copy link
Contributor Author

Wow, this is amazing!

The es6 classes will probably be a bigger challenge, but this already should already meet the big goal of allowing people to use what they want.

I'll comment on the individual issues.

@kum-deepak
Copy link
Collaborator

Please create an ES6 label, so that all issues can easily be grouped.

@gordonwoodhull
Copy link
Contributor Author

Done. 👍

@kum-deepak
Copy link
Collaborator

Github would not allow me to add label to issues. Thanks for doing it!

@kum-deepak
Copy link
Collaborator

Regarding refactoring, my thoughts so far:

  • Put the code into multiple folders, say, core, mixins, and charts.
  • Move ChartRegistery as a class into separate module.
  • Move Config as a class and shift some global variables into it.
  • Utils as a module (or class with all static functions).
  • Consider Errors and Filters to be converted to class hierarchies.
  • Move base mixin as a class (probably not named a mixin but an abstract class)
  • Evaluate other mixins - can any of these be converted to a helper(s), if nothing works there is http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/
  • focus on method reassignments.

Please share your views.

@gordonwoodhull
Copy link
Contributor Author

Thanks @kum-deepak!

I hadn't thought about changing the directory structure, but most of these ideas make sense.

I think I read that Error was improved in ES6 to make it easier to extend. It might be a good time to look at #1521, since I don't think the errors work consistently across browsers.

I'm not sure if Filters can be converted to classes, since they are mostly annotated arrays. Derived from Array? Maybe worth a try.

You are welcome to try, but I think you will be disappointed trying to make the mixins work as mixins. Historically they were just base classes, and functionally that's all they really are. #432 changed the names to "mixin" but you have never been able to mix and match them or really use them as mixins in any way. It's caused confusion when people tried to e.g. add capping to bar charts.

What does "focus on method reassignments" refer to?

@gordonwoodhull
Copy link
Contributor Author

btw, I bumped up your privileges so you should be able to deal with issues better. you might have to accept an invite.

@kum-deepak
Copy link
Collaborator

I will go onto simpler things first 😄

I realize that few places methods are reassigned (as in monkey patch), I just wanted to keep a reminder for me to be careful around those.

@kum-deepak
Copy link
Collaborator

Regarding Exceptions - I feel that if we just extend the two custom exceptions from Error, it should simply work. I will test it soon.

@privateOmega
Copy link

privateOmega commented Dec 27, 2019

@gordonwoodhull @kum-deepak I would be happy to pitch-in if you guys need some extra manpower on this.

@kum-deepak
Copy link
Collaborator

Welcome @privateOmega. Please check all items labeled ES6 .

Quite a lot of work has happened, you can checkout https://github.com/dc-js/dc.js/tree/es6 and play around.

@privateOmega
Copy link

privateOmega commented Dec 27, 2019

@kum-deepak I had a quick look and it seems only the README review issue is open as of now.

I have build a table package using ES6 branch of dc.js. Please take a look when convenient and also Could you please point me in the direction to make use of existing upstream as well till you guys release ES6 version?

@gordonwoodhull
Copy link
Contributor Author

IMO 4.0 is ready for release.

I just want to merge the elasticX change first. Have not found time for that due to my day job.

Sorry for the delay. Hope to get this out by end of year.

@privateOmega
Copy link

privateOmega commented Dec 27, 2019

@gordonwoodhull Could you please point out what's to be done? If it's something basic, I can do that for you.

Is it this one you are referring to?

@gordonwoodhull
Copy link
Contributor Author

Thanks for the offer. Yes that is the issue.

If you would give the branch a test that would be a big help. However, I have not ported it to the es6 branch because I think I saw some performance issues.

The performance issue is what I need to track down and I also need to do some more testing and maybe write an automated test or two.

@privateOmega
Copy link

@gordonwoodhull Yes, I tried the "focus dynamic interval" example that you mentioned and it seems it is indeed slow. But I am clueless as to how to identify what causes the performance issue.

@gordonwoodhull
Copy link
Contributor Author

I decided not to hold things up any longer.

I am releasing v4 without the elasticX change. We will maintain v3 for a while, and we can continue to do fixes using the support/3.x branch

@gordonwoodhull
Copy link
Contributor Author

@gordonwoodhull
Copy link
Contributor Author

Did not receive any reports, so dc.js 4.0 is released.

Amazing work @kum-deepak!

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

No branches or pull requests

3 participants