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@v6 with dc@v4 #1749

Closed
wants to merge 17 commits into from
Closed

d3@v6 with dc@v4 #1749

wants to merge 17 commits into from

Conversation

kum-deepak
Copy link
Collaborator

Ref #1748

Steps so far:

  • Updated dependency on d3@v6
  • Removed hardcoded d3 module dependencies.
  • Added d3-collection as a dependency.
  • It turns out if we add d3-colllection@1.0.7 with d3@v6, dc remains happy.

For dc@v5 we can remove d3-collection properly if we decide that it will only work with d3@v6.

@gordonwoodhull
Copy link
Contributor

I did a quick review. I like the general approach.

I wonder if we could simplify this further, and encapsulate the backward-compatibility code, by moving the logic detecting if d3.event exists into cpt()?

This would mean consistently using cpt() even for calls to _brushing and _onZoom and so on.

I am hoping this would mean we only do the ugly if(event) check in one place, and it might make it easier to remove d3@5- support in the future. (First change it to the identity function, and then remove it entirely.) More importantly, it will make our event handlers purely compliant with d3@6 standards.

I would also prefer to remove the new dependency on d3-collection, since that library is deprecated. TBH I haven't used d3.group / d3.rollup yet but I understand they cover the functionality of d3.nest.

@kum-deepak kum-deepak changed the title Test - D3@v6 with dc@v4 d3@v6 with dc@v4 Aug 28, 2020
@kum-deepak
Copy link
Collaborator Author

Makes sense. Let me try.

@kum-deepak
Copy link
Collaborator Author

Please check d899c5c and e8f80fa

@kum-deepak
Copy link
Collaborator Author

For d3.nest a middle ground seems to be working. Please check 012ae33

@kum-deepak
Copy link
Collaborator Author

We also seem to be using d3.set. Let me get onto that.

@kum-deepak
Copy link
Collaborator Author

kum-deepak commented Aug 29, 2020

d3.set is used only in specs. During the conversion, stumbled across a subtle difference in behavior vis-a-vis ES6 Set:

const set = d3.set();
set.add('3');
set.has(3); // true, false with ES6 set

This happens as d3.set always converts keys to strings. On the other hand, ES6 Set allows a key to be a number and considers 3 and '3' to be different.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Aug 30, 2020

Rebased and merged to develop. Thanks @kum-deepak!

I also took the opportunity to do an npm update and audit fix.

These are my remaining concerns before release. I am not sure if these matter, just want to be careful.

  1. package.json now refers to d3-collection in its devDependencies and the old references to the rest of the specific d3 modules are gone. I'm not sure if these changes were just for testing, or meant to be permanent.
  2. Ideally d3-collection will not be used if d3 is version 6. However, we still import nest from it in d3compat.js, and don't use it. Not sure if this will cause trouble, or how to avoid it. (Dynamic module load, shudder?)

@kum-deepak
Copy link
Collaborator Author

1. package.json now refers to d3-collection in its `devDependencies` and the old references to the rest of the specific d3 modules are gone. I'm not sure if these changes were just for testing, or meant to be permanent.

This is intentional, we now depend on whatever d3 depends on. Sometimes compiler or linter may warn if we import from a dependency that is not directly added to package.json. However, no issues otherwise.

2. Ideally d3-collection will not be used if d3 is version 6. However, we still import `nest` from it in `d3compat.js`, and don't use it. Not sure if this will cause trouble, or how to avoid it. (Dynamic module load, shudder?)

In ES6 version, if we remove it, it should still compile and work, though I wanted to keep it clean. It is not used during testing as it is not copied into spec/3rd-party. Also, I have ensured that the code checks for .gorups, so, that in case both are available .groups gets used.

In summary, no harm in keeping it.

@gordonwoodhull
Copy link
Contributor

I see, it's just a change of strategy about how we deal with d3 modules. Makes sense!

I'll go ahead and release this.

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

Successfully merging this pull request may close these issues.

2 participants