feat: isolate d3js compatility layers

[nomego]

Enable es module users to specify d3js compatibility layer while
maintaining dynamic adaptation to run-time d3js version for UMD users.

TODO:

• html-legend has a hard dependency on d3v5 selection
• possibly have a better import than 'dc/src/core/..' for es module users
• fix units.js:47 Uncaught (in promise) Error: dc.units.ordinal should not be called - it is a placeholder
• move compat helpers out of config to d3compat to avoid clash with upcoming dc@5 config

Fixes #1822
Fixes #1823

[kum-deepak]

Thanks! I have had a brief look. In general, I like the approach. In addition to the changes, we should add a CI task to run the tests with dc@5.

I have only one feedback for now. In the next version (branch dc-v5) we are looking to move config to a simple object that can be set by end-users. So, I will suggest that we do not latch this object onto config. Will it work if we just created an object d3Compat exported from dc-compat.js.

[kum-deepak]

I have created a new PR #1825, it adds the Github CI test for d3@5. Please rebase onto this branch, it will help during the development process.

[nomego]

Ok rebased on https://github.com/dc-js/dc.js/tree/d3-v5-test

Also, interestingly, as you could see in dbb7542, the d3@5-style eventhandler (minus the d3-selection event) was needed to make the tests pass (even before rebasing) so both types of events are present in a d3@v6 context. The comment was very misleading and should be updated.

Another interesting question is the html-legend hard dep on d3-selection event (d3@5 - https://github.com/dc-js/dc.js/pull/1824/files#diff-534c34a8d11372d1866f6cf7246f8c408e75018ed8a21bd8d5692be1d21df8c8R2) - even though it's commented out, none of the tests fail. Is this due to missing tests or superfluous configuration?

[gordonwoodhull]

Rollup is producing some new warnings on this branch:

(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
d3 (imported by src/core/d3compat-v5.js, src/core/d3compat-v6.js)
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
d3 (guessing 'd3')
d3 (guessing 'd3')

I think the inconsistent event parameters have to be a bug & we should only be seeing one call signature at a time... looking into it...

[gordonwoodhull]

Okay, I looked at this a little bit. The issue is that we didn't port all of our code and all of our tests to the d3@6 event argument signature.

The previous adaptHandler was too lax because it allowed our code to be inconsistent, using d3@5 arguments some places and d3@6 arguments in other places.

It's a pain, but I recommend dropping the last commit and making event handling consistent throughout dc.js. I'm willing to help, but my time is limited so it might take a little while.

It is especially annoying if we are testing d3@5 because the tests will have to fake events using the d3@5 or d3@6 event argument signature, conditionally.

@kum-deepak, do you agree with this assessment? I remember hoping at the time that the adaptor would make it easy for us to drop d3@5 compatibility in the future, but instead it has left us halfway ported.

[nomego]

@gordonwoodhull I agree that it would be a good opportunity to fix the event handling if this is an unintentional behavior.
I reverted the fix for now and if you change your mind we can rather drop the revert. Or drop both once we're getting out of draft status for the PR.

I also pushed a fix for the rollup warning.

[kum-deepak]

The code in html-legend.js is incorrect, legend.js has the correct version. We need to apply the same code in html-legend.js.

@gordonwoodhull, I agree, we should fix the event handlers so that it works without being lax.

[gordonwoodhull]

Nice to see the tests clearly succeed on d3@5 and fail on d3@6. This is the right change.

[nomego]

@kum-deepak Pushed a commit to use legend.js logic for keydown in html-legend.js, please review.

@gordonwoodhull @kum-deepak Will you guys push fixes for the event handling?

[kum-deepak]

@nomego I will like to push changes, however, I am getting permission errors.

[kum-deepak]

The same change needed for focus and blur handlers

[nomego]

I did select to allow access for you:

But now I've invited you guys to my forked repo as well.

[kum-deepak]

Thanks, @nomego, I am able to push now.

For fixing event invocation code from specs, I was thinking of introducing helper functions that will take the original handler and parameter.

[gordonwoodhull]

Okay... I would prefer to use d3@6-style handlers so the code is more clear for people who are up with the latest D3.

But I recognize this a lot more work, so sure, let's just get it working again.

Maybe we can finally clean it up when we rip out the d3@5 compatibility layer someday.

[nomego]

Looked like you guys merged the d2-v5-test branch so I rebased this on upstream develop branch and also dropped the eventHandler revert to fix in a separate PR if I understood your comments correctly.

[nomego]

Also moved the compat layer away from config according to @kum-deepak 's comments about the upcoming dc@5 config approach. I think this is ready for review/merge?

[nomego]

Oh by the way, my sample app (https://github.com/nomego/dc-owc) totals 200KB when built, using BarChart, LineChart and CompositeChart from dc.js along with crossfilter2 and needed d3.js parts. For comparison, the minified versions of the libs total approx 400KB so it's at least a 50% decrease in size, considering that the app itself is included in the 200KB but not the 400KB.
I pushed the dc.js es module usage changes to the repo if you're interested.

[gordonwoodhull]

Thank you. That is great news about the size reduction.

This is a clean solution and I like that everything is resolved at initialization time rather than runtime.

I still want to revert the eventHandler and fix the tests and code, but we can handle that on our side before merging.

@kum-deepak, please give it another review.

[nomego]

@gordonwoodhull IMHO the PR is already of decent size, implementation is complete, behavior is unchanged and all tests pass. It would be great to get this merged to properly support es module usage, create a new PR and drop the d3v5 event behavior (line) from the d3v6-compat layer and fix all the event handling issues there - it would also make that PR a nice scope for a separate type of change.
Of course you guys make the call but it would be a shame to see this PR scope creep and drag out for long before getting merged.

[gordonwoodhull]

I understand your concern and promise not to delay too long. If @kum-deepak does not add fixes, and I don't find time to do so myself, I will merge as-is within a week.

[kum-deepak]

I have gone through the changes again. In my opinion, let us merge it. We can take the spec cleanup task in the future - maybe even in the next version.

[gordonwoodhull]

You guys are right; we should never hold up a new feature due to the discovery of old bugs. Also, this PR fixes the bugs which were in the core library; there are only bugs in the tests remaining.

Released as 4.2.6 and I'm going to take a look at the tests now.

[gordonwoodhull]

Added d3compat.callHandler and adapted all tests to use it instead of calling event handlers directly.

Now all tests pass with eventHandler properly specialized for each version.

This also unearthed an extra call to d3compat.eventHandler, which is no longer idempotent. Fixed in 74d2c58