new(geo, vendor): add `d3-geo@3` to `visx-vendor`, support `geoPath.digits(n)` #1767

kachkaev · 2023-11-09T22:45:56Z

🚀 Enhancements

Closes #1761

update d3-geo to 3.0.8 to support geoPath.digits(n)
move d3-geo dep to @visx/vendor because the new version is ESM-only

Demo

`digits={5}`

`digits={0}`

no `digits` prop (defaults to 3)

kachkaev · 2023-11-09T22:47:30Z

package.json

@@ -115,6 +115,7 @@
    "ts-node": "9.1.1",
    "typescript": "^3.8.3"
  },
+  "packageManager": "yarn@1.22.19",


I am using corepack on my computer, which comes with the latest Yarn by default (v4). Specifying packageManager helps corepack pick the right version and run yarn install without errors.

packages/visx-geo/src/projections/Projection.tsx

kachkaev · 2023-11-09T23:04:46Z

packages/visx-geo/src/projections/Projection.tsx

+  // @see https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/67363
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-call
+  if (digits !== undefined) path.digits(digits);
+  if (pointRadius !== undefined) path.pointRadius(pointRadius);


Side suggestion / question: https://github.com/airbnb/visx/pull/213/files#r1388679342

I think this change is fine (even though 0 shouldn't be a valid input)

williaster

Hey @kachkaev this looks great to me overall! No issues from my end except with the one visx/demo typescript change I mention (it does look like CI is failing on that so I would try reverting the change and seeing if that fixes it)

williaster · 2023-11-10T02:17:23Z

packages/visx-demo/tsconfig.json

@@ -14,11 +14,12 @@
    "moduleResolution": "node",
    "composite": true,
    "outDir": "lib",
-    "resolveJsonModule": false,
+    "resolveJsonModule": true,


ah, I think there's an issue where next forces this when running the demo app locally, but I think it fails the TS generation in CI. let's see if it passes with this change, else I'd have you revert it for merging (and separately need to fix the issue)

I reverted this accidental change in f7dddef. Yeah the file was updated by Next when I ran these commands:

cd packages/visx-demo NODE_OPTIONS=--openssl-legacy-provider yarn dev

(I had to use NODE_OPTIONS to avoid Error: error:0308010C:digital envelope routines::unsupported in Node 18.18.2)

It would be nice to update packages/visx-demo/tsconfig.json but I guess we can do this in a separate DX-focused PR.

packages/visx-geo/src/projections/Projection.tsx

williaster · 2023-11-10T02:22:58Z

packages/visx-geo/src/projections/Projection.tsx

+  // @see https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/67363
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-call
+  if (digits !== undefined) path.digits(digits);
+  if (pointRadius !== undefined) path.pointRadius(pointRadius);


I think this change is fine (even though 0 shouldn't be a valid input)

packages/visx-geo/src/projections/Projection.tsx

williaster · 2023-11-10T02:32:46Z

jest.config.js

@@ -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)|delaunator|internmap|robust-predicates)/)',
+    'node_modules/(?!(d3-(array|color|format|geo|interpolate|scale|time|time-format)|delaunator|internmap|robust-predicates)/)',


thanks for finding this – sorry I missed it in my instructions! this isn't ideal to have to do for each ESM-only package, will think on ways to streamline it.

Co-authored-by: Chris Williams <williaster@users.noreply.github.com>

kachkaev · 2023-11-10T12:37:07Z

Thanks for your feedback @williaster! I updated the diff, hope the CI will pass this time 🤞

I guess what’s left is PR description, unless you have other suggestions on the diff. If you can help me with triaging the changes by type (breaking change, feature, etc.), that’d be great. I can look at other PRs for examples, but there is quite a lot going on in this small PR at once (major bump of d3-geo, moving it to the vendor package, adding a new prop). So I might find it hard to find a good match and you’ll likely need to correct my description anyway. I guess it goes directly to CHANGELOG by the looks of it.

williaster · 2023-11-10T21:33:16Z

Thanks @kachkaev 🙌 I took a pass at updating the PR title + description. Feel free to tweak if you like, only the title is added to the change log (the PR labels determine what it is labeled as there. I think this is just a minor version bump / new feature since it shouldn't be a breaking change for consumers – even though the d3-geo dep is changing behind the scenes it should only enable net-new functionality)

williaster · 2023-11-10T21:42:15Z

CI passed and Happo diffs look fine!

You can't really see diffs by eye, but it's picking up on some (as expected going from random digits to the new default of 3)

williaster · 2023-11-10T22:58:03Z

I think unless you have any other things you want to add, this is probably good to go. Will wait to hear from you before merging/releasing, tho!

kachkaev · 2023-11-10T23:37:38Z

Oh great! Thanks for helping! If the diff and the description look acceptable, let’s get this PR in!

Visx is a great lib! I’m glad I’ve been able to pay back with this small contribution! 🙌

kachkaev · 2023-11-12T16:55:50Z

~~I guess we can wait for DefinitelyTyped/DefinitelyTyped#67384 and remove #1767 (comment)~~

UPD: Done in a3f86cb

williaster

this is great, thanks for landing the type change as well!! 🎉

really appreciate the contribution and hard work. glad to hear you are enjoying visx overall :)

github-actions · 2023-11-13T20:13:31Z

🎉 This PR is included in version v3.5.0 of the packages modified 🎉

…igits(n)` (airbnb#1767) * Update `d3-geo`, add it to `visx-vendor`, support `Projection#digits` * Link to issue * Reduce diff * tsconfigs * Update packages/visx-geo/src/projections/Projection.tsx Co-authored-by: Chris Williams <williaster@users.noreply.github.com> * Reduce diff * Update `@types/d3-geo` to `3.1.0` * Dedupe `d3-array` --------- Co-authored-by: Chris Williams <williaster@users.noreply.github.com>

Update d3-geo, add it to visx-vendor, support Projection#digits

e121423

kachkaev commented Nov 9, 2023

View reviewed changes

Link to issue

bcb9396

kachkaev commented Nov 9, 2023

View reviewed changes

packages/visx-geo/src/projections/Projection.tsx Outdated Show resolved Hide resolved

kachkaev mentioned this pull request Nov 9, 2023

Fix center and add pointRadius, innerRef #213

Merged

kachkaev commented Nov 9, 2023

View reviewed changes

kachkaev mentioned this pull request Nov 9, 2023

Support geoPath.digits(n) in geo projection components #1761

Closed

Reduce diff

a74d30b

williaster reviewed Nov 10, 2023

View reviewed changes

williaster added 🚀enhancement @visx/geo @visx/vendor minor-release labels Nov 10, 2023

kachkaev and others added 3 commits November 10, 2023 09:21

tsconfigs

b580716

Update packages/visx-geo/src/projections/Projection.tsx

dd98ec5

Co-authored-by: Chris Williams <williaster@users.noreply.github.com>

Reduce diff

f7dddef

williaster changed the title ~~Update d3-geo, add it to visx-vendor, support geoPath.digits(n)~~ new(geo, vendor): add d3-geo@3 to visx-vendor, support geoPath.digits(n) Nov 10, 2023

williaster marked this pull request as ready for review November 10, 2023 22:57

kachkaev added 2 commits November 12, 2023 19:21

Update @types/d3-geo to 3.1.0

a3f86cb

Dedupe d3-array

7c665a7

williaster approved these changes Nov 13, 2023

View reviewed changes

williaster merged commit 1ee0374 into airbnb:master Nov 13, 2023
2 checks passed

kachkaev deleted the visx-geo-digits branch November 13, 2023 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(geo, vendor): add `d3-geo@3` to `visx-vendor`, support `geoPath.digits(n)` #1767

new(geo, vendor): add `d3-geo@3` to `visx-vendor`, support `geoPath.digits(n)` #1767

kachkaev commented Nov 9, 2023 •

edited

Loading

kachkaev Nov 9, 2023 •

edited

Loading

kachkaev Nov 9, 2023

williaster Nov 10, 2023

williaster left a comment

williaster Nov 10, 2023

kachkaev Nov 10, 2023 •

edited

Loading

williaster Nov 10, 2023

williaster Nov 10, 2023

kachkaev commented Nov 10, 2023 •

edited

Loading

williaster commented Nov 10, 2023

williaster commented Nov 10, 2023

williaster commented Nov 10, 2023

kachkaev commented Nov 10, 2023 •

edited

Loading

kachkaev commented Nov 12, 2023 •

edited

Loading

williaster left a comment

github-actions bot commented Nov 13, 2023

new(geo, vendor): add d3-geo@3 to visx-vendor, support geoPath.digits(n) #1767

new(geo, vendor): add d3-geo@3 to visx-vendor, support geoPath.digits(n) #1767

Conversation

kachkaev commented Nov 9, 2023 • edited Loading

🚀 Enhancements

Demo

digits={5}

digits={0}

no digits prop (defaults to 3)

kachkaev Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

kachkaev Nov 9, 2023

Choose a reason for hiding this comment

williaster Nov 10, 2023

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

williaster Nov 10, 2023

Choose a reason for hiding this comment

kachkaev Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

williaster Nov 10, 2023

Choose a reason for hiding this comment

williaster Nov 10, 2023

Choose a reason for hiding this comment

kachkaev commented Nov 10, 2023 • edited Loading

williaster commented Nov 10, 2023

williaster commented Nov 10, 2023

williaster commented Nov 10, 2023

kachkaev commented Nov 10, 2023 • edited Loading

kachkaev commented Nov 12, 2023 • edited Loading

williaster left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2023

new(geo, vendor): add `d3-geo@3` to `visx-vendor`, support `geoPath.digits(n)` #1767

new(geo, vendor): add `d3-geo@3` to `visx-vendor`, support `geoPath.digits(n)` #1767

kachkaev commented Nov 9, 2023 •

edited

Loading

`digits={5}`

`digits={0}`

no `digits` prop (defaults to 3)

kachkaev Nov 9, 2023 •

edited

Loading

kachkaev Nov 10, 2023 •

edited

Loading

kachkaev commented Nov 10, 2023 •

edited

Loading

kachkaev commented Nov 10, 2023 •

edited

Loading

kachkaev commented Nov 12, 2023 •

edited

Loading