search

dc-js / dc.js

Multi-Dimensional charting built to work natively with crossfilter rendered with d3.js
Apache License 2.0
7.42k stars 1.81k forks source link

feat: isolate d3js compatility layers #1824

Closed nomego closed 3 years ago

nomego commented 3 years ago

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

TODO:

Fixes #1822 Fixes #1823

kum-deepak commented 3 years ago

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 commented 3 years ago

I have created a new PR https://github.com/dc-js/dc.js/pull/1825, it adds the Github CI test for d3@5. Please rebase onto this branch, it will help during the development process.

nomego commented 3 years ago

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

Also, interestingly, as you could see in https://github.com/dc-js/dc.js/pull/1824/commits/dbb7542df6429306cd82c0055249a48897a05862, 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

nomego commented 3 years ago

@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 commented 3 years ago

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

nomego commented 3 years ago

I did select to allow access for you: image

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

kum-deepak commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 74d2c58c