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

d3@6 has been released #1748

Closed
gordonwoodhull opened this issue Aug 24, 2020 · 14 comments
Closed

d3@6 has been released #1748

gordonwoodhull opened this issue Aug 24, 2020 · 14 comments
Assignees

Comments

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Aug 24, 2020

dc@4 may already be compatible; D3’s embrace of ES6 was our rationale to move on in #1520.

Or there may be other breaking changes in the API.

Now there is a pre-release PR d3/d3#3438 so we should test it!

@kum-deepak
Copy link
Collaborator

Tested briefly on develop branch. It is failing, at least two changes that affect:

  • It seems the way event is passed into the zoom type of calls has changed.
  • d3.nest has been removed.

Could not find any upgrade guide, if it has not been released yet, we can wait.

@kum-deepak
Copy link
Collaborator

Changes are deeper, there are more failures. I think we should wait for the official upgrade guide. For example one of the failures:

Expected '−20' to be '-20'

It seems d3 now uses UTF8 (226, 136, 146) which looks like -. This was from test case related to Axis.

@gordonwoodhull gordonwoodhull self-assigned this Aug 24, 2020
@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Aug 24, 2020

The changes are all documented in the release notes for the individual libraries, and I have been following the progress. I think I know what these problems mean:

Maybe it doesn't matter, but I'd like to be compatible before d3@6 is released. I'm willing to do the work (as long as I find the time).

@kum-deepak
Copy link
Collaborator

The hyphen issue should only affect test cases, so that should be easiest. This should not affect backward compatibility.

I had created an issue for d3.nest #1720. It should be doable in a backward-compatible way.

The events related change - I remember when converting for d3v4, I wished if the event was not to be picked up from a global variable, as it was impacting nested events (e.g., in range/focus charts). This should make implementation simpler. It will impact out test infrastructure, where we simulate events. Not sure if it can be done backward compatible without writing conditional code. We will know when we dig deeper.

@gordonwoodhull
Copy link
Contributor Author

Good points.

I guess there are two kinds of backward compatibility to be concerned about:

  • compatibility with existing user code - as far as I can tell, these changes shouldn't break user code
  • compatibility with d3@5 - I agree, we probably need some conditional code, both for events and for d3.nest. d3@5 does not contain the replacement group, rollup functions as these were introduced in d3-array@2 which is ES6 only.

Conditional code is annoying but probably better than a hard break. The alternative, making a hard break to d3@6 with dc@5, would be simpler code but would probably confuse users.

(I am also not sure how we detect the difference, given that we import individual d3 modules and don't have access to the overall D3 version. But I'm sure there is a way.)

Definitely more complicated than I thought!

@gordonwoodhull
Copy link
Contributor Author

FWIW, CHANGES.md is now available on the above PR.

As far as I can tell, the three changes you identified are the only ones that affect us (although we use d3.event for a lot more than just d3-zoom).

@kum-deepak
Copy link
Collaborator

kum-deepak commented Aug 26, 2020

I have been experimenting, the d3.event change is the only one that is going to be complex - both for dcv4 and dcv5. In d3.event they have also removed .sourceEvent (which was hacky underneath in d3 code). However, this was used in dc to avoid infinite recursion - when the event was fired because of programmatic activity, .sourceEvent was undefined.

We will need to figure out a new way for focus/range charts.

@gordonwoodhull gordonwoodhull changed the title d3@6 is available for testing d3@6 has been released Aug 27, 2020
@gordonwoodhull
Copy link
Contributor Author

I know that I want to release version 4.1 with compatibility with both d3@5 and d3@6. I should have time to help with this in the next week.

I don’t have a strong opinion if dc@5 should remain compatible with d3@5 - up to you.

@kum-deepak
Copy link
Collaborator

Some good news - I have, hopefully, been able to avoid infinite recursion on zoom/brush. The code has been written to work with both d3@v5 and d3@v6.

kum-deepak@006f8f4 does it in more robust way even for d3@v5. kum-deepak@81d38cd makes it compatible with both d3@v5 and d3@v5.

@kum-deepak
Copy link
Collaborator

Next challenge - d3@v5 used to pass d, i to event handlers, d3@v6 seems to be passing evt, d. There seem to be a few cases that actually use i. Reviewing usage as of now.

@gordonwoodhull
Copy link
Contributor Author

Nods, I think we may have used this strategy in the past.

There is often a way to refactor code to avoid infinite recursion by logic, but it takes deep thought and everyone has to remember what the rules are. This more blunt approach is effective and easy to understand.

Kudos for the try/catch - that's the Achilles heel of this approach. I think your approach is also safe in case _withoutBrushEvents is itself called recursively, since you save the last state in a closure not on the object.

@kum-deepak
Copy link
Collaborator

I think we have all individual steps under control. I have written a method in utils to wrap event handlers and provides them with correct value:

export function cpt (handler) {
    return function (a, b) {
        if (a.currentTarget) {
            // d3@v6 - b is __data__, a is the event
            handler.call(this, b, a);
        } else {
            // older d3 - a is __data__
            handler.call(this, a);
        }
    }
}

I wanted the method name to be small, so currently using cpt, please suggest alternatives if you do not like it.

I reviewed all the places we had i, only PieChart actually needed it. By sheer luck, PieChart passes the output of d3.pie, so, it already had d.index.

I have sequenced the commits to have all dc changes, so, that we can test those against d3@v5 as well as d3@v6. Specs and examples I am assuming to work with only one version.

I will create two PRs, one containing commits covering dc changes only.

We need to test against both d3 versions.

@kum-deepak
Copy link
Collaborator

Created a PR #1750 with the updated dc code configured against d3@v5. We need to test the updated code against both d3@v5 and d3@v6.

@gordonwoodhull
Copy link
Contributor Author

Implemented in #1749 and released in 4.1.0.

Thanks @kum-deepak!!

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

No branches or pull requests

2 participants