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

Use const/let instead of var #212

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

domoritz
Copy link
Contributor

Merge after #210

@curran

@Fil
Copy link
Member

Fil commented Jul 30, 2020

Using const and let adds almost 200 bytes (1%) to the compressed script.

16142 d3-scale.min.js (var)
16328 d3-scale.min.js (const-let)

I've tried collapsing vars (let x,y,z…) but terser is already doing it, and returns the same file.

For the sake of science I tried to replace all const by let, and the resulting size was 16156 bytes (+0.1%). [Of course we would not do that since it defeats the purpose of using these precise const/let declarations.]

(EDIT: when comparing the gzip'ed minified, we're talking of a difference of 20 bytes.)

Copy link
Contributor

@curran curran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I love how it makes the code more clear in the sense that you know what can be expected to change with reassignment vs. not.

@curran
Copy link
Contributor

curran commented Aug 24, 2020

This change is great. Merging this PR would set a good precedent and open the door for future PRs that upgrade var to let and const across all of D3.

@domoritz
Copy link
Contributor Author

domoritz commented Feb 6, 2021

Do you still plan to merge this pull request now that d3 6 is out?

Fil added a commit that referenced this pull request Sep 19, 2021
(see also #212)
Fil
Fil previously approved these changes Sep 19, 2021
@Fil Fil dismissed their stale review September 19, 2021 12:13

human error! sorry

mbostock pushed a commit that referenced this pull request Sep 19, 2021
(see also #212)
mbostock added a commit that referenced this pull request Sep 19, 2021
* fix #234; exact log ticks

* let/const

(see also #212)

Co-authored-by: Philippe Rivière <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants