-
Notifications
You must be signed in to change notification settings - Fork 721
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
deps: bump all dev config #1301
Conversation
Size Changes
View raw build statsPrevious (master){
"visx-annotation": {
"esm": 28965,
"lib": 39026
},
"visx-axis": {
"esm": 20970,
"lib": 25370
},
"visx-bounds": {
"esm": 2842,
"lib": 3264
},
"visx-brush": {
"esm": 54984,
"lib": 58922
},
"visx-chord": {
"esm": 3459,
"lib": 4688
},
"visx-clip-path": {
"esm": 4421,
"lib": 5978
},
"visx-curve": {
"esm": 323,
"lib": 1464
},
"visx-demo": {
"esm": 0,
"lib": 0
},
"visx-drag": {
"esm": 6751,
"lib": 8815
},
"visx-event": {
"esm": 3797,
"lib": 5172
},
"visx-geo": {
"esm": 13259,
"lib": 16519
},
"visx-glyph": {
"esm": 14893,
"lib": 19789
},
"visx-gradient": {
"esm": 17800,
"lib": 22517
},
"visx-grid": {
"esm": 18922,
"lib": 22784
},
"visx-group": {
"esm": 1619,
"lib": 2246
},
"visx-heatmap": {
"esm": 7286,
"lib": 8622
},
"visx-hierarchy": {
"esm": 12266,
"lib": 18076
},
"visx-legend": {
"esm": 26999,
"lib": 34033
},
"visx-marker": {
"esm": 8962,
"lib": 11197
},
"visx-mock-data": {
"esm": 326005,
"lib": 329416
},
"visx-network": {
"esm": 4546,
"lib": 6706
},
"visx-pattern": {
"esm": 11779,
"lib": 15910
},
"visx-point": {
"esm": 819,
"lib": 1094
},
"visx-react-spring": {
"esm": 13660,
"lib": 18210
},
"visx-responsive": {
"esm": 21901,
"lib": 26961
},
"visx-scale": {
"esm": 18452,
"lib": 29710
},
"visx-shape": {
"esm": 86057,
"lib": 107037
},
"visx-stats": {
"esm": 13911,
"lib": 15494
},
"visx-text": {
"esm": 8518,
"lib": 10981
},
"visx-threshold": {
"esm": 2911,
"lib": 3820
},
"visx-tooltip": {
"esm": 14147,
"lib": 20413
},
"visx-visx": {
"esm": 1467,
"lib": 4243
},
"visx-voronoi": {
"esm": 2286,
"lib": 3005
},
"visx-wordcloud": {
"esm": 2624,
"lib": 4431
},
"visx-xychart": {
"esm": 169667,
"lib": 236421
},
"visx-zoom": {
"esm": 15606,
"lib": 18482
}
} Current{
"visx-annotation": {
"esm": 29032,
"lib": 40229
},
"visx-axis": {
"esm": 20859,
"lib": 25340
},
"visx-bounds": {
"esm": 3028,
"lib": 3450
},
"visx-brush": {
"esm": 56089,
"lib": 60231
},
"visx-chord": {
"esm": 3486,
"lib": 4716
},
"visx-clip-path": {
"esm": 4502,
"lib": 6062
},
"visx-curve": {
"esm": 323,
"lib": 1464
},
"visx-demo": {
"esm": 0,
"lib": 0
},
"visx-drag": {
"esm": 6751,
"lib": 9001
},
"visx-event": {
"esm": 3807,
"lib": 5200
},
"visx-geo": {
"esm": 13425,
"lib": 16705
},
"visx-glyph": {
"esm": 15109,
"lib": 20013
},
"visx-gradient": {
"esm": 18135,
"lib": 22853
},
"visx-grid": {
"esm": 19057,
"lib": 22924
},
"visx-group": {
"esm": 1646,
"lib": 2274
},
"visx-heatmap": {
"esm": 7414,
"lib": 8770
},
"visx-hierarchy": {
"esm": 12209,
"lib": 18111
},
"visx-legend": {
"esm": 27079,
"lib": 34279
},
"visx-marker": {
"esm": 9101,
"lib": 11337
},
"visx-mock-data": {
"esm": 326005,
"lib": 329416
},
"visx-network": {
"esm": 4690,
"lib": 6869
},
"visx-pattern": {
"esm": 11826,
"lib": 15957
},
"visx-point": {
"esm": 819,
"lib": 1094
},
"visx-react-spring": {
"esm": 13780,
"lib": 18784
},
"visx-responsive": {
"esm": 22467,
"lib": 27919
},
"visx-scale": {
"esm": 18512,
"lib": 30389
},
"visx-shape": {
"esm": 86210,
"lib": 108359
},
"visx-stats": {
"esm": 13830,
"lib": 15432
},
"visx-text": {
"esm": 8510,
"lib": 11240
},
"visx-threshold": {
"esm": 2911,
"lib": 3820
},
"visx-tooltip": {
"esm": 14465,
"lib": 21476
},
"visx-visx": {
"esm": 1467,
"lib": 4429
},
"visx-voronoi": {
"esm": 2313,
"lib": 3033
},
"visx-wordcloud": {
"esm": 2661,
"lib": 4673
},
"visx-xychart": {
"esm": 169936,
"lib": 244090
},
"visx-zoom": {
"esm": 15541,
"lib": 18621
}
} |
e5dc2f8
to
21f8b82
Compare
21f8b82
to
495d720
Compare
@@ -74,9 +74,10 @@ | |||
"@testing-library/user-event": "^11.0.1", | |||
"chalk": "4.1.0", | |||
"coveralls": "^3.0.6", | |||
"enzyme": "^3.10.0", |
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.
nit: the ending space is unnecessary.
I will open a PR soon to convert the rest of enzyme tests into RTL. So we can get rid of enzyme from dev dependency : )
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.
sweeeeeeet re enzyme! that will be amazing 🦾
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.
a few comments, but stamping to unblock.
one thing i've found useful for these types of PRs in the future is to have 3 different commits added to the PR. The first one is all the config/package.json changes. The second is after running yarn install and autofixing everything. Then the third commit contains all manual changes. That way a reviewer can look at the first and third commits only for review, and ignore the second. food for thought!
@@ -37,7 +37,7 @@ export function getDomainFromExtent( | |||
const invertedEnd = scaleInvert(scale, end + (end < start ? -tolerentDelta : tolerentDelta)); | |||
const minValue = Math.min(invertedStart, invertedEnd); | |||
const maxValue = Math.max(invertedStart, invertedEnd); | |||
if (scale.invert) { | |||
if ('invert' in scale && typeof scale.invert !== 'undefined') { |
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.
shouldn't this remain if ('invert' in scale && scale.invert) {
? Not sure what types invert can be
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 think this is just the way TS wants to check for whether it's defined. it's either a function or undefined
if (source) ribbon.source(source); | ||
if (target) ribbon.target(target); | ||
if (radius) setNumberOrNumberAccessor(ribbon.radius, radius); | ||
if (startAngle) setNumberOrNumberAccessor(ribbon.startAngle, startAngle); | ||
if (endAngle) setNumberOrNumberAccessor(ribbon.endAngle, endAngle); | ||
const path = (ribbon(chord) as unknown) as string | null; | ||
const path = ribbon(chord) as unknown as string | null; |
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.
do we still need as unknown
?
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.
yeah 😢
); | ||
}; | ||
export default LinkTypesPage; | ||
import React from 'react'; |
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.
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.
lol, no idea
@@ -17,7 +17,8 @@ describe('sclaeRadial()', () => { | |||
|
|||
it('set unknown', () => { | |||
const scale = scaleRadial({ domain: [0, 10], unknown: 'green' }); | |||
expect(scale('sandwich' as any)).toEqual('green'); | |||
// coerce to make TS happy | |||
expect(scale('sandwich' as unknown as number)).toEqual('green'); |
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.
wait, what?
scale
seems to only expect a number. you're passing in sandwich
and forcing typescript to be ok with it. Shouldn't this test be deleted since TS makes sure this can never happen?
Or, if you want to test the unknown values, can we pass in a number that's not in the domain to test that? either -1
or 11
?
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 think although TS would def catch it, we wanted it to be robust for folks who don't use TS. I'd rather keep this as originally implemented since I only altered the type casting here.
thanks for the review @etr2460 !
💯 I agree this is ideal. I think there was an issue here where auto-fixing actually introduced a lot of |
🎉 This PR is included in version |
🏠 Internal
This PR
nimbus
config we currently useimport type { ... }
syntax@visx/scale
to@visx/brush
because we used made upScale
types inbrush
which I could no longer get to work@gazcn007 if this is merged before #1268, there will likely be merge conflicts because of some prettier updates. they shouldn't be too crazy, though. We can try and get yours in before this as well if you'd rather.
@kristw @gazcn007