-
Notifications
You must be signed in to change notification settings - Fork 233
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 v7 support #380
base: master
Are you sure you want to change the base?
d3 v7 support #380
Conversation
805903b
to
f2ce3fa
Compare
@danielcaldas Is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome that you tackle this one @lironhl 👍 Let's further discuss these changes, and see if we can keep backward compatibility with d3 v5
@@ -181,7 +181,10 @@ export default class Graph extends React.Component { | |||
.on("end", this._onDragEnd); | |||
|
|||
d3Select(`#${this.state.id}-${CONST.GRAPH_WRAPPER_ID}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the changes are minimal would that be a way of:
- Use duck typing (or similar mechanism) to infer at runtime the d3 version that the client is using
- Based on the version determined in
step 1.
, use the proper APIs ford3Select
and other affected integration points.
If the above is not possible, we could potentially consider the following:
- Add a new config to
graph.config.js
where the user can specify the major version ofd3
used. - Use the client-defined
config.d3MajorVersion
property to adapt our integration points withd3
.
I would like to keep retro compatibility with d3 v5
if possible 🙏 . If we see this is too much trouble, I'll consider a major release of react-d3-graph
so that we can upgrade our peer dep. on d3 to v6
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielcaldas This is probably not that hard to do, the code will look something like this:
import { ..., event as d3V5Event } from "d3-selection";
// code
_zoomed = d3V6Event => {
const d3Event = d3V5Event || d3V6Event;
// code
}
-
I think supporting
d3 v5
will definitely increase the code complexity in this package. -
As you said the changes were minimal, so the users of our package won't have a hard time migrating, plus I don't think
that the vast majority of our users, implemented a complex d3 logic, outside of our package, which means they'll only need to update their dependency in thepackage.json
to migrate to the nextreact-d3-graph
major release. -
It'll take a considerable effort to support both of the d3 versions, and I don't think d3 maintainers are going to support the
d3 v5
anymore (with bug fixes, features, etc), so we'll put an effort into a d3 version which may not be in the best condition over time.
I think you should consider a major release, anyway let me know what you think about what I wrote, If we'll get to the conclusion that we should support d3 v5
I'll gladly add the support for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes total sense, I'm considering pushing this awesome change of yours in a release candidate version for react-d3-graph@3.0.0
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielcaldas I think releasing react-d3-graph@3.0.0
, with this PR and my other PRs (#379 and #378), is a great choice 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lironhl Let's focus on the others first. They're not breaking changes; hence we can still release them within a 2.x
version. The strategy would be to get as much stuff as possible compatible withing 2.x
. Thanks for the awesome work 👏
4844bdd
to
e243e64
Compare
Whats the ETA for this release @danielcaldas ? |
058fdd0
to
7f154e2
Compare
@danielcaldas @antoninklopp guys a whole new major version of d3 was released and this PR wasn't merged 😆. Resources for this PR--env=jsdom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great to me @lironhl !
@danielcaldas, I think that we should start planning for a v3 release (since you suggested that this change would be part of a v3) as a lot of people might already have moved to d3 v6 and it contains breaking changes for react-d3-graph
We might want to start with a beta?
332ac1e
to
c73a2ee
Compare
c73a2ee
to
baf9a98
Compare
closes #365, This PR adds support for d3 v6!
Resources