bvaughn / react-virtualized-auto-sizer

Standalone version of the AutoSizer component from react-virtualized
MIT License
613 stars 82 forks source link

Is it possible to test AutoSizer-powered components in unit tests? #69

Closed kaiyoma closed 8 months ago

kaiyoma commented 1 year ago

I know, I know, this has been asked and answered before, but I'm finding that all the answers I've uncovered are several years old and are unsatisfactory. Using the last comment of this issue as a reference:

You can pass in an explicit width/height props (for testing only) ...

I would argue this is philosophically a bad approach, for several reasons:

  1. It leads to "props creep" (API gets more complex)
  2. It makes the component more complicated
  3. It gives users a false illusion that they can set static height/width values, when this isn't actually intended

Basically, the component is being tainted to accommodate unit test shortcomings, which feels like a perversion of the intent of the code.

... and use those as the defaultHeight and defaultWidth for AutoSizer when present

Even if you don't agree with the philosophy above, and plow ahead anyway, you'll find that this doesn't work. Here's a snippet of my component:

    <AutoSizer defaultWidth={500} style={{ height: '100%', position: 'relative', width: '100%' }}>
      {({ height, width }) => {
        console.log('🚀 ~ file: Chart.tsx:97 ~ width:', width);

Here's what I see when I run my test:

🚀 ~ file: Chart.tsx:97 ~ width: NaN
🚀 ~ file: Chart.tsx:97 ~ width: NaN

NaN is considered defined, so the default isn't used.

Or you can mock out the portion of the browser API that AutoSizer users to measure itself

This also doesn't work. If you try it, you get this:

TypeError: Cannot assign to read only property 'offsetHeight' of object '[object Object]'

I've also heard the argument of "AutoSizer isn't meant to be tested, so refactor your components accordingly" and sure, we could, but again, this feels like a maintenance burden brought on by testing shortcomings. The better approach is to mock at the test level and leave the components alone.

Looking at everything from a 10,000-foot view, isn't the real issue here that AutoSizer is passing NaN? That behavior seems extremely undesirable (what caller would actually want that?), so why not just fix it inside AutoSizer? Treat NaN like undefined (so that the defaults get used) and all these problems go away.

bvaughn commented 1 year ago

Want to submit a PR + proposal?

kaiyoma commented 1 year ago

Want to submit a PR + proposal?

I've never looked through the innards of the code, but I could take a stab at it.

kaiyoma commented 1 year ago

For anyone else stumbling upon this, here's my workaround for now. If you have a low number of components or unit tests, then maybe this is an acceptable low-effort fix:

    <AutoSizer>
      {({ height, width }) => {
        // `height` and `width` will be NaN in unit tests
        // https://github.com/bvaughn/react-virtualized-auto-sizer/issues/69
        const heightNum = !height || _.isNaN(height) ? 100 : height;
        const widthNum = !width || _.isNaN(width) ? 100 : width;
janeklb commented 1 year ago

FWIW we've been mocking the auto-sizer module as follows.

type AutoSizerModule = typeof import('react-virtualized-auto-sizer');

jest.mock<AutoSizerModule>('react-virtualized-auto-sizer', () => ({
  __esModule: true,
  ...jest.requireActual<AutoSizerModule>('react-virtualized-auto-sizer'),
  default: jest.fn().mockImplementation(({ children }) => {
    return (children as (size: { width: number; height: number }) => ReactNode)({
      width: 600,
      height: 600,
    });
  }),
}));

Aside from the smelly typing (children as (size: { width: number; height: number }) => ReactNode), it works fine.

kaiyoma commented 1 year ago

BTW, I'm actually encountering this NaN problem in UI code, not just unit tests, so it seems that there's a fundamental bug somewhere in AutoSizer. When I render an auto-sized component, I see this in the browser console (which is causing my e2e tests to fail):

Warning: Received NaN for the `width` attribute. If this is expected, cast the value to a string.

The width is coming directly from AutoSizer. I think AutoSizer should be fixed (in its core code) to not ever pass NaN, and to maybe even replace that value with undefined, so that default values take over correctly.

bvaughn commented 8 months ago

I think this issue was addressed in 28723d154513b92fd797a712fa03307c5495e6a9 and published in 1.0.22

Closing this for now but we can reopen if needed.

janeklb commented 8 months ago

Setting doNotBailOutOnEmptyChildren={true} allows me to remove all react-virtualized-auto-sizer mocks i have in my tests.

The bail out is documented as an optimisation but it has these unexpected (from a user perspective) side-effects in tests, as well as remounts (as seen in #85). What do you think of making this the default behaviour, and allow people to opt-in to the optimisation instead of opting-out?

Conversely (or maybe just additionally..), even if you think it's best to have the default on, what do you think about renaming the the escape hatch prop to bailOutOnEmptyChildren (with it defaulting to true)? https://github.com/bvaughn/react-virtualized-auto-sizer/pull/87

bvaughn commented 8 months ago

I think the current default behavior, which was the intended default behavior all along, should remain.