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

Replace var with let or const as appropriate. #3435

Closed
curran opened this issue Jun 15, 2020 · 21 comments
Closed

Replace var with let or const as appropriate. #3435

curran opened this issue Jun 15, 2020 · 21 comments

Comments

@curran
Copy link
Contributor

curran commented Jun 15, 2020

I'm curious, what the stance is in terms of adopting let and const in this repo?

Possible approaches:

  • Consistently ES5 (var only)
  • Consistently ES6 (e.g. would a PR be welcome that converts all var to let & const?)
  • Allow internal inconsistency (mixed var, let, const within & between files)

This relates to how PRs should be reviewed with respect to usage of these keywords. Thanks!

@Fil
Copy link
Member

Fil commented Jun 15, 2020

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 d3/d3-geo#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.)

@curran
Copy link
Contributor Author

curran commented Jun 15, 2020

Nice! lebab looks like a great approach. Inconsistent state seems fine - thanks for confirming.

@domoritz
Copy link

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.

@Fil Fil transferred this issue from d3/d3-array Jun 24, 2020
@curran
Copy link
Contributor Author

curran commented Jun 25, 2020

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.

@mbostock
Copy link
Member

It’s not a goal at this time to rewrite everything. We’re focused on getting 6.0 released.

@Fil Fil mentioned this issue Jul 29, 2020
@Fil
Copy link
Member

Fil commented Jul 30, 2020

@domoritz has done the manual work of replacing var declarations with the appropriate const and let for d3-scale (d3/d3-scale#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.

@domoritz
Copy link

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.

@Fil
Copy link
Member

Fil commented Jul 30, 2020

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" 👍

@domoritz
Copy link

Great. To me, that's within the noise.

@anlexN
Copy link

anlexN commented Aug 13, 2020

please rewrite d3 in es2015+ syntax

@curran
Copy link
Contributor Author

curran commented Aug 13, 2020

lol PRs welcome.

@domoritz
Copy link

I sent one here: d3/d3-scale#212 ;-)

@curran
Copy link
Contributor Author

curran commented Dec 20, 2020

I think it makes sense to use ES6 in the documentation sample code provided in the README for all D3 packages.

@mbostock mbostock changed the title Stance on var vs. let & const? Replace var with let or const as appropriate. Feb 6, 2021
@mbostock
Copy link
Member

mbostock commented Feb 6, 2021

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.

@domoritz
Copy link

domoritz commented Feb 6, 2021

@lgrammel has been working on a bot that could automate a lot of the work.

@Fil
Copy link
Member

Fil commented Feb 6, 2021

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.

@lgrammel
Copy link

lgrammel commented Feb 6, 2021

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.

@domoritz
Copy link

domoritz commented Feb 6, 2021

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.

@Fil
Copy link
Member

Fil commented Feb 6, 2021

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.

@d9j
Copy link

d9j commented Oct 12, 2022

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

@mbostock
Copy link
Member

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.

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

No branches or pull requests

7 participants