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

Cleanup .call/.apply #1582

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Conversation

kum-deepak
Copy link
Collaborator

I have been reviewing all usage of .call and .apply. Majority of such calls are to invoke some d3 method which expect specific value of this.

There are some cases within dc code as well. Majority of these set this to a d3 element. In general I am finding code difficult to understand and maintain with .call/.apply.

I have restructured a function in fb3f9db. In this now the d3-element is passed a parameter and this is set to the chart. This makes the function very similar to other functions.

I think the same restructuring will help in cleaning up getColor in color-mixin.

This is for your review and discussion, then I will convert similar cases in other charts.

@gordonwoodhull
Copy link
Contributor

This makes a lot of sense in thecboxMenu, since

  • the function can use this where it used to use _chart
  • the function is completely private and changing it won't break the public interfacs

You'll find the same function in selectMenu since cbox was based on select.

Of course the baseMixin case is very clear too; that falls out of storing private members on the chart object instead of the closure.

I wonder if the other cases are as clear-cut. getColor is definitely more complicated, since it is called both with the chart and the layer as this (the element as well?) And the bar chart and line chart have different layer objects IIRC.

I think it was just a lazy shortcut someone took, but it's going to be tricky to prove it. We can't rely 100% on the tests for that one, since it could silently break a color that isn't tested.

And since it's public, it needs to be documented in an upgrade manual. Presumably it affects colorCalculator as well.

I completely agree that using this for anything other than the chart is going to be confusing in this new ES6 world. Yet we do have the D3 precedent to contend with, and it doesn't look like that will change: d3/d3#2246 (comment)

So I'm cautiously optimistic about this approach and I'd like to review the other cases.

@kum-deepak kum-deepak added the ES6 label Sep 23, 2019
@kum-deepak
Copy link
Collaborator Author

I have not touched getColor so far. Other than that I have cleaned up. Please review.

Merge it if you find it is sufficient for now. For getColor I will create a separate PR.

@gordonwoodhull
Copy link
Contributor

Completely agree - all of these free functions are much improved as member functions.

Thank you for your careful, conscientious work, @kum-deepak!

@gordonwoodhull gordonwoodhull merged commit 2ff47f3 into dc-js:es6 Sep 25, 2019
@kum-deepak kum-deepak deleted the cleanup-call-apply branch September 28, 2019 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants