fix: Improve typing in @visx/responsive enhancers #1783

paolostyle · 2024-01-08T23:09:23Z

💥 Breaking Changes

Technically for TypeScript it does contain a breaking change, you can currently manually pass parentWidth/parentHeight /screenWidth/screenHeight prop to the component wrapped in withParentSize/withScreenSize. Now whether that makes any sense is arguable as it basically invalidates entire purpose of the HOC. The actual runtime behavior did not change, it's just that now TypeScript will scream at you if you try to do this. I can roll this change back if you feel like this is too big of a change.

🚀 Enhancements

Exported WithParentSizeProvidedProps and WithScreenSizeProvidedProps to make it easier to properly type props of the component to be enhanced.

📝 Documentation

Added documentation regarding undocumented config options (i.e. debounceTime, enableDebounceLeadingCall and their counterparts in withScreenSize, initialWidth, initialHeight) to the readme
Updated the examples to make it more clear how to properly use it with TypeScript

🐛 Bug Fix

Currently it is not possible to use withParentSize and withScreenSize HOCs on a component that contains required props different from the ones injected by the HOC without some ugly hacks. Example:

import { withParentSize } from '@visx/responsive';

type Props = {
  data: number[];
  parentWidth?: number;
  parentHeight?: number;
};

const Chart = ({
  data, parentWidth, parentHeight,
}: Props) => {
  return null;
};

export default withParentSize(Chart); // error!
// error: Property 'data' is missing in type 'WithParentSizeProps & WithParentSizeState' but required in type 'Props'.ts(2345)

// to make it work we'd have to:
// import { WithParentSizeProps } from '@visx/responsive/lib/enhancers/withParentSize';
// ...
// export default withParentSize<Props & WithParentSizeProps>(Chart);

There is no reason why this shouldn't be allowed, so this PR fixes it (i.e. the code above now transpiles correctly). I adjusted a test for this HOC as well to make it more clear that passing other props is now allowed. It also fixes #1645.

🏠 Internal

I did not change the actual implementation but I'm happy to contribute useParentSize and useScreenSize hooks in a separate MR which I believe would be a bit easier to use. These two HOCs could then use these hooks internally instead.
I dropped defaultProps as it makes typing way more difficult and in this particular case it can be easily replaced by ?? operator, it's used in only one place.

williaster

Thanks for the contribution @paolostyle 🙏 This looks great overall, had just a couple minor comments.

I did not change the actual implementation but I'm happy to contribute useParentSize and useScreenSize hooks in a separate MR which I believe would be a bit easier to use. These two HOCs could then use these hooks internally instead.

Definitely would love to see this! agree hooks are more useful these days (visx is showing it's age 🤭)

💥 Breaking Changes: Technically for TypeScript it does contain a breaking change, you can currently manually pass parentWidth/parentHeight /screenWidth/screenHeight prop to the component wrapped in withParentSize/withScreenSize. Now whether that makes any sense is arguable as it basically invalidates entire purpose of the HOC. The actual runtime behavior did not change, it's just that now TypeScript will scream at you

thanks for noting this. also wanted to say I agree this is okay to change as this isn't desired behavior, I wouldn't mark this as a breaking version change.

packages/visx-responsive/Readme.md

packages/visx-responsive/test/withParentSize.test.tsx

packages/visx-responsive/src/enhancers/withScreenSize.tsx

packages/visx-responsive/src/enhancers/withParentSize.tsx

packages/visx-responsive/src/types/index.ts

packages/visx-responsive/test/withParentSize.test.tsx

paolostyle · 2024-01-12T00:28:11Z

Thanks for the review! I'll try to contribute the hooks when I have some free time 😊

paolostyle · 2024-02-18T15:13:42Z

@williaster I updated the PR with useParentSize and useScreenSize hooks. useParentSize is mostly extracted from <ParentSize> component, useParentSize mostly from withParentSize. Some additional notes:

I did not refactor enhancers to use the hooks. I initially did that, but then I realized that the library currently still supports React <16.8 versions, so that would be a breaking change.
There's a bunch of things I marked as deprecated and frankly I'd really like to get rid of them sooner than later, but as far as I can tell all the changes are backwards compatible
I added a new way of defining ResizeObserver polyfill, inspired by one of the comments in some merged PR (Would love to credit the person that suggested it but I can't find it anymore...). LMK what do you think. Previous ways of passing it (through props/arguments) are still there.
Some cosmetic changes to readme
I wanted to add more comprehensive tests but I just couldn't make it work at all, especially with fake timers and parts that were touching ResizeObserver. At this point I think it's just because of the very old version of Jest and/or JSDom, I tried to copy/paste some tests from the @juggle/resize-observer package and they weren't functional (i.e. they were passing, but changing checked values to something else did not break them when it should).

Slightly offtopic - are there some blockers I should be aware of if I wanted to update some very old dependencies (TypeScript, Jest, JSDOM)?

williaster

Hey @paolostyle thanks for taking another pass at all of this and adding the hooks! it looks great overall 😎

I had a few relatively minor comments, and I think there's a breaking change export that's failing CI. good catch withreact <16.8 support, we use hooks in some packages but mark ^16.8 in the peer dependencies. I wonder if this even compiles now for folks on 16, even if they don't explicitly use the hooks 🤔 I think because they were there before this change, it's okay.

I don't think there are major things to note regarding older infra that we have. lmk if you hit any issues when tinkering or refactoring, though.

packages/visx-responsive/src/enhancers/withScreenSize.tsx

williaster · 2024-02-21T22:38:01Z

packages/visx-responsive/src/enhancers/withScreenSize.tsx

-      enableDebounceLeadingCall: true,
-    };
-
+    displayName = `withScreenSize(${


thank you for improving this as well 🙏

williaster · 2024-02-21T22:41:01Z

packages/visx-responsive/test/useScreenSize.test.ts

+    expect(result.current).toEqual({ width: 1280, height: 1024 });
+  });
+
+  test('it should update the screen size on window resize', () => {


thank you for adding these!

After the changes I had to adjust it and use real timers, Jest fake timers pre-v26 are absolutely unusable and frankly I don't have any interest in figuring out why they don't work - they were replaced for a reason. I wanted to upgrade Jest to the current version and then these tests were behaving as expected but then there are tons of TypeScript errors when building other packages (because of the old TypeScript version). Like I mentioned I would like to upgrade the dependencies across visx packages so consider it a temporary change.

packages/visx-responsive/src/hooks/useParentSize.ts

packages/visx-responsive/src/hooks/useScreenSize.ts

packages/visx-responsive/src/resizeObserver.ts

paolostyle · 2024-02-22T00:46:55Z

good catch withreact <16.8 support, we use hooks in some packages but mark ^16.8 in the peer dependencies. I wonder if this even compiles now for folks on 16, even if they don't explicitly use the hooks 🤔 I think because they were there before this change, it's okay.

So just to make sure I understand correctly - do you want to me to adjust the enhancers implementations to use the hooks? I have the code that uses the hooks stashed so that doesn't require any effort but just want to make sure that's what you mean.

williaster · 2024-02-22T04:47:11Z

So just to make sure I understand correctly - do you want to me to adjust the enhancers implementations to use the hooks? I have the code that uses the hooks stashed so that doesn't require any effort but just want to make sure that's what you mean.

sorry for not being clear. I just verified that if you deep import withParentSize using react@16 (no hook support), that the app compiles okay. so just to be safe, I would leave the enhancer implementations as they are / don't refactor to hooks in case there is usage like that out there.

…properties usage

paolostyle · 2024-02-22T15:27:15Z

Updated the PR, I think there shouldn't be any build errors and I addressed your feedback.

paolostyle · 2024-03-07T20:39:23Z

Shameless bump - @williaster I would really appreciate if you could take a look at the PR, thanks!

williaster

thanks for the ping @paolostyle 🙏 and again for all the hard work and iteration here ❤️ lgtm, should be released soon 🚀

github-actions · 2024-03-08T19:02:31Z

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

dumbmatter · 2024-03-08T22:01:38Z

packages/visx-responsive/src/index.ts

+export { default as withParentSize, WithParentSizeProvidedProps } from './enhancers/withParentSize';
+export { default as withScreenSize, WithScreenSizeProvidedProps } from './enhancers/withScreenSize';
+export { default as useParentSize, UseParentSizeConfig } from './hooks/useParentSize';
+export { default as useScreenSize, UseScreenSizeConfig } from './hooks/useScreenSize';


These lines seem to be causing an error in TypeScript (if you have it scan node_modules) because those new types are not exported from the transpiled versions of these 4 files.

Ah damn. Sorry about that. Created a PR that should fix this issue.

oof, thank you. also found a blind spot in our CI, thanks for the quick report and fix 🙏

williaster added 🐛 bug 🚀enhancement @visx/responsive minor-release labels Jan 11, 2024

williaster reviewed Jan 11, 2024

View reviewed changes

paolostyle added 3 commits February 18, 2024 15:57

improve responsive enhancers typing

7ac5d96

review changes

8101893

add useParentSize and useScreenSize hooks

5a187ee

paolostyle force-pushed the responsive-typing branch from 7c2f140 to 5a187ee Compare February 18, 2024 14:58

williaster reviewed Feb 21, 2024

View reviewed changes

revert resize observer changes, add initialSize props, review window …

07946b9

…properties usage

williaster approved these changes Mar 8, 2024

View reviewed changes

williaster merged commit ed9bb2f into airbnb:master Mar 8, 2024
2 checks passed

dumbmatter reviewed Mar 8, 2024

View reviewed changes

paolostyle mentioned this pull request Mar 8, 2024

fix: explicit type exports in @visx/responsive #1809

Merged

hshoff mentioned this pull request Mar 25, 2024

Create hooks for @vx/responsive #485

Closed

darthmaim mentioned this pull request May 25, 2024

Add return type to withBoundingRects #1837

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve typing in @visx/responsive enhancers #1783

fix: Improve typing in @visx/responsive enhancers #1783

paolostyle commented Jan 8, 2024

williaster left a comment

paolostyle commented Jan 12, 2024

paolostyle commented Feb 18, 2024 •

edited

Loading

williaster left a comment

williaster Feb 21, 2024

williaster Feb 21, 2024

paolostyle Feb 22, 2024

paolostyle commented Feb 22, 2024 •

edited

Loading

williaster commented Feb 22, 2024

paolostyle commented Feb 22, 2024

paolostyle commented Mar 7, 2024

williaster left a comment

github-actions bot commented Mar 8, 2024

dumbmatter Mar 8, 2024

paolostyle Mar 8, 2024

williaster Mar 9, 2024

fix: Improve typing in @visx/responsive enhancers #1783

fix: Improve typing in @visx/responsive enhancers #1783

Conversation

paolostyle commented Jan 8, 2024

💥 Breaking Changes

🚀 Enhancements

📝 Documentation

🐛 Bug Fix

🏠 Internal

williaster left a comment

Choose a reason for hiding this comment

paolostyle commented Jan 12, 2024

paolostyle commented Feb 18, 2024 • edited Loading

williaster left a comment

Choose a reason for hiding this comment

williaster Feb 21, 2024

Choose a reason for hiding this comment

williaster Feb 21, 2024

Choose a reason for hiding this comment

paolostyle Feb 22, 2024

Choose a reason for hiding this comment

paolostyle commented Feb 22, 2024 • edited Loading

williaster commented Feb 22, 2024

paolostyle commented Feb 22, 2024

paolostyle commented Mar 7, 2024

williaster left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2024

dumbmatter Mar 8, 2024

Choose a reason for hiding this comment

paolostyle Mar 8, 2024

Choose a reason for hiding this comment

williaster Mar 9, 2024

Choose a reason for hiding this comment

paolostyle commented Feb 18, 2024 •

edited

Loading

paolostyle commented Feb 22, 2024 •

edited

Loading