d3
d3 copied to clipboard
d3
Replace var with let or const as appropriate.
I'm curious, what the stance is in terms of adopting let and const in this repo?
Possible approaches:
- Consistently ES5 (
varonly) - Consistently ES6 (e.g. would a PR be welcome that converts all
vartolet&const?) - Allow internal inconsistency (mixed
var,let,constwithin & between files)
This relates to how PRs should be reviewed with respect to usage of these keywords. Thanks!
I don't know, but I like the precision and code safety (and scoping) given by let/const. However it's true that it could be a long time during which the code base would be inconsistent.
If we wanted to semi-automate the migration process we could use lebab (converse of babel). See the experiment I did on d3-geo at https://github.com/d3/d3-geo/issues/180 — some good came out of it, in particular a 3% smaller file size. If you look at the commits you can see the lebab "transforms" applied one by one, to help figure out a bit better what's possible. (There were suprisingly few manual steps.)
Nice! lebab looks like a great approach. Inconsistent state seems fine - thanks for confirming.
I think if the goal is to converge towards ES6, some internal inconsistency is fine. Maybe you can have something in the guidelines to ask people to gradually update to ES6 in their pull requests.
Moving everything at once is usually only something that core developers can do since you need very quick merges (to avoid conflicts) and some knowledge of how things fit together.
Let's lebab d3/d3-*/**/*.js!
Here's a question: would PRs be welcome that just migrate individual files to ES6?
Maybe one file (or few files) at a time is the safest way to do it.
It’s not a goal at this time to rewrite everything. We’re focused on getting 6.0 released.
@domoritz has done the manual work of replacing var declarations with the appropriate const and let for d3-scale (https://github.com/d3/d3-scale/pull/212). One issue is that the resulting (compressed) file size has increased by 1%.
To be clear I think it's a good idea to have the more precise code; we just have to be a bit cautious. Some changes allowed by ES6 syntax are also going to bring gains in download times.
Is the 1% more after or before gzip compression?
To me, 1% more is a good trade off for safer code. Also, you could run Babel over the code to remove the let/const in the released code.
Ah, right! I had measured the minified's size—but you're correct that what we should measure is the gzip'ed minified: then it's not even 1%, but 0.4% "heavier" 👍
I think it makes sense to use ES6 in the documentation sample code provided in the README for all D3 packages.
The eventual goal is to replace var with let or const, to adopt arrow functions, and otherwise to adopt ES2015+ language features as appropriate. For D3 6.0 we focused on the user-visible aspects of this (namely supporting iterables, and encouraging the use of arrow functions as event listeners). Eventually it’d be nice to update the internals as well, but I’m a little wary of churn and have plenty of other stuff to work on that I think is more pressing.
@lgrammel has been working on a bot that could automate a lot of the work.
My experiment with lebab was quite positive, too, so I believe a bot could do the work without creating too many new issues by itself. However note we currently have 138 pull-requests open throughout the D3 project, and a large change like this might impact every single one of them, creating random conflicts. I'd rather we spend our collective time on evaluating, rejecting and merging those PRs.
I'd love to help with the adoption of new ES2015+ language features. I could tune/modify the bot ( https://p42.ai/ ) to fit the needs of D3 exactly, so all you'd need to do would be to generate/review bot pull requests.
The final decision is with you of course. I just wanted to share my experience.
Consistent code style (e.g. through automatic formatting) and splitting variable declarations (from one line for multiple declarations to one line per variable) could reduce churn in the long run. We had pretty good experiences with Prettier+ESlint in the Vega project where formatting reduced the number of conflicts (and updating prs with the right format is straight forward as well). Automated formatting also reduced our burden as reviewers of external prs since we didn't have to ask for style changes anymore.
If there's a way to upgrade the PRs, that objection vanishes. I am totally sold on the benefits of using ES6 and prettier throughout. But I think Mike doesn't like prettier as much as I do.
i know it's off topic. but want to thank you guys for this library. literally saved my ass. few times.. using it in combination with other chart libraries.. using d3js for customization of third party vendors svg graphs where api for customization is not provided by vendors
We don’t have any immediate plans to do this, and don’t need a tracking issue for it. Maybe we’ll do it eventually.