Create new @visx/delaunay package #1678
Conversation
|
Hey @williaster, if you have some time can you take a look at this review? The code and documentation closely follow @visx/voronoi, so hopefully there aren't too many issues. |
There was a problem hiding this comment.
Hey @SheaJanke thanks for the contribution and fix for the OOM issue! Sorry this has taken me forever to review but I just left some comments which pertain to the new @visx/vendor package (which unfortunately also resulted in some merge conflicts). after those changes I think we should be able to land this/I'll keep an eye on it.
Thanks for all the work adding a new package and finding all of the "hooks" to add references, @visx/demo examples, etc. 🙏
|
Thanks for the review! I'll try to address the comments and merge conflicts in the next week or so. |
* Change d3-delaunay references to @visx/vendor package. * Delete unnecessary yarn.lock file and visx-demo.html
There was a problem hiding this comment.
@SheaJanke this is great, thanks for such a clean addition and pattern matching everything so well 💯
also appreciate the additional fixes for the @visx/vendor merge conflicts. I had a couple super minor suggestions but overall this LGTM. let me know if you want to work on those minor things, else we can land as is.
jest.config.js
Outdated
| @@ -44,6 +44,6 @@ module.exports = { | |||
| verbose: false, | |||
| testPathIgnorePatterns: ['<rootDir>/packages/visx-demo'], | |||
| transformIgnorePatterns: [ | |||
| 'node_modules/(?!(d3-(array|color|format|interpolate|scale|time|time-format)|internmap)/)', | |||
| 'node_modules/(?!(d3-(array|color|delaunay|format|interpolate|scale|time|time-format)|delaunator|internmap|robust-predicates)/)', | |||
There was a problem hiding this comment.
ah interesting, I missed this in the esm/cjs work. I might need to revisit this (in a separate PR)
There was a problem hiding this comment.
I had another look at this, and I was able to remove d3-delaunay from here. The other two still seem to be necessary.
There was a problem hiding this comment.
Thanks for looking at this. I did some more digging and it seems like delaunator and robust-predicates may be ESM-only which likely trips up jest.
I pulled and built your branch locally and you can see that this generated/vendored CJS file references an the ESM-only delaunator lib. I'm not sure why that doesn't mess up our next demo app, perhaps it's all resolving to the ESM versions so this path isn't triggered.
all this to say that if someone hits CJS/ESM issues with this we might have to also vendor delaunator + robust-predicates in the future, but since it seems to work with next.js as-is I think we can merge this for now.
|
happo looks good also 👍 |
|
Thanks for the feedback! I made the requested changes 👍 |
|
thanks again for all the hard work here @SheaJanke ! 🙌 |
|
🎉 This PR is included in version |
🚀 Enhancements
closes: #1330