-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Charts port to ESM #496
Charts port to ESM #496
Conversation
Hmm, seems I can't set this PR to be ready for review until @tlwr signs the new CLA. Would you mind doing so Toby? |
I have signed the CLA, sorry for delaying you! |
No problem! Thanks 😊
…On Sun, 10 Mar 2019, 13:30 Toby Lorne, ***@***.***> wrote:
I have signed the CLA, sorry for delaying you!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#496 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACAWftKyvOSKoSRgYpZWP1DeFMEeK_Mwks5vVQkSgaJpZM4bNJY->
.
|
a0767f9
to
ca6d472
Compare
src/core/operations/HeatmapChart.mjs
Outdated
* @license Apache-2.0 | ||
*/ | ||
|
||
import * as d3 from "d3"; |
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.
Would we get a benefit from only importing specfic symbols here? @n1474335 added a bundle size analyser that I think we can use to check
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.
I've had a quick look, and based on this I'm not sure it will be possible. I've given it a shot using @std/esm and the configure scripts don't seem too happy.
When I run this fork and attempt to create a heatmap with input
I get the following message:
Is this how it should be run? Some simple nightwatch tests to verify that charts get created could also be handy to verify that the chart is created? I appreciate that testing the output of these operations would be fragile using the traditional test framework. What do you think? |
Great work @artemisbot and @tlwr. Thanks very much for working hard on this one, it's great to see it in at last! |
All credit goes to @artemisbot thanks for getting this over the line, over 18months later |
At long last, we will have #143 in CyberChef! I intend to port the operations already written first, and I may take a look at the extras that were suggested in the old pull request once those are done.