Version 1.0.22 is breaking all of our unit tests #80

kaiyoma · 2024-02-13T21:00:38Z

If I upgrade from 1.0.20 to 1.0.22, all of our component unit tests start failing because elements are no longer visible. I think it has to be this change causing the problems: 28723d1

@saperdadsk Do you know why this would be the case?

The text was updated successfully, but these errors were encountered:

kaiyoma · 2024-02-13T21:06:28Z

This is essentially what I'm seeing now in our component unit tests:

TestingLibraryElementError: Unable to find an element by ...

Ignored nodes: comments, script, style
<body>
  <div>
    <div
      style="height: 200px; width: 200px;"
    >
      <div
        style="overflow: visible; width: 0px;"
      />
    </div>
  </div>
</body>

AutoSizer is rendering an empty shell with zero width, and none of its children are appearing.

kaiyoma · 2024-02-13T21:11:34Z

Some more potentially valuable data points:

the commit message for the possibly offending commit says that tests were not included 😬
the same commit message references jsdom, but not everyone uses it (we use happy-dom for these tests)

saperdadsk · 2024-02-13T21:24:41Z

Hmmm... @kaiyoma it's not obvious to me how that change would cause your tests to fail - it should only come into effect if window.getComputedStyle was previously returning an empty string. Could your tests previously have ignored an error coming from the lib?

If you put a breakpoint in the relevant code, what do you see window.getComputedStyle(this._parentNode) return for style.padding?

Tests were not included for the specific issue fixed by change, but existing tests did pass...

saperdadsk · 2024-02-13T21:28:52Z

(The mention of JSDOM is the commit message was in reference to the JSDOM version used for testing in this repo - these changes "should" not care what library you use to render your component)

kaiyoma · 2024-02-13T21:37:34Z

I'm happy to help, but I think I need more guidance here. Where exactly am I supposed to put that call? If I put it right before our AutoSizer call, I get this:

TypeError: Cannot read properties of undefined (reading '_parentNode')

If I put it inside the render function inside AutoSizer, nothing ever prints.

saperdadsk · 2024-02-13T21:42:01Z

@kaiyoma I would put a breakpoint at the beginning of the function _onResize inside the AutoSizer library, and step untl you get to

        // Guard against AutoSizer component being removed from the DOM immediately after being added.
        // This can result in invalid style values which can result in NaN values if we don't handle them.
        // See issue #150 for more context.

        const style = window.getComputedStyle(this._parentNode) || {};

which is where the change was.

saperdadsk · 2024-02-13T21:45:07Z

(Specifically, inside the code in the file in (likely) node_modules/react-virtualized-auto-sizer/dist that your code is consuming)

kaiyoma · 2024-02-13T21:48:56Z

~~Hmm, I'm having trouble getting that to work. We use ESM, so I added statements to the ESM file, but they're not working. I tried adding statements to the CJS file too, but that's also not working.~~

Got it to work after restarting Vitest. Here's what I see when I console log the style variable:

CSSStyleDeclaration { parentRule: null }

saperdadsk · 2024-02-13T21:54:11Z

@kaiyoma What does style.paddingLeft return? If it's "", then you probably were ignoring the error before... Or maybe happy-dom allows for setting NaN values to style props and just ignores them during render or something?

kaiyoma · 2024-02-13T21:58:13Z

Yes, style.paddingLeft is the empty string. We're not ignoring any errors in our unit tests though; any errors would cause our tests to fail. I'm more inclined to think that this is due to a difference between happy-dom and jsdom.

kaiyoma · 2024-02-13T22:03:46Z

Hmm, even if I switch to jsdom, I'm still seeing basically the same kind of failure. (Actually, the markup looks worse and more incomplete.)

kaiyoma · 2024-02-13T22:06:05Z

If I manually edit the AutoSizer library and change the 4 || operators back to ??, then my unit tests go back to passing, so it's definitely that change.

bvaughn · 2024-02-13T22:08:59Z

If I manually edit the AutoSizer library and change the 4 || operators back to ??, then my unit tests go back to passing, so it's definitely that change.

That is surprising to me.

@kaiyoma What does style.paddingLeft return? If it's "", then you probably were ignoring the error before... Or maybe happy-dom allows for setting NaN values to style props and just ignores them during render or something?

This would also be my assumption, based on the other things mentioned in this thread.

kaiyoma · 2024-02-13T22:09:48Z

@bvaughn At this point, I'm actually seeing the same behavior between happy-dom and jsdom. Both fail with this recent change, and both pass without the change.

bvaughn · 2024-02-13T22:10:21Z

Can you package this up in a small runnable repro and share it here?

saperdadsk · 2024-02-13T22:12:36Z

@kaiyoma What version of react-dom are your tests running with, and do you run with development mode on? I think the message I was seeing was from react-dom (i.e https://github.com/facebook/react/blob/dc3178151b8cb0359e5c36b8ed57458b807ae8e3/packages/react-dom-bindings/src/shared/warnValidStyle.js#L115). I wonder if your tests are suppressing the message or something?

kaiyoma · 2024-02-13T22:13:13Z

Can you package this up in a small runnable repro and share it here?

Not quickly/easily. This is a large repo of proprietary code, so I would have to spend a non-trivial amount of time whittling it down.

bvaughn · 2024-02-13T22:14:14Z

Can you package this up in a small runnable repro and share it here?

Not quickly/easily. This is a large repo of proprietary code, so I would have to spend a non-trivial amount of time whittling it down.

That's fair. I think we'd probably be able to help a lot more efficiently if we could view the "failing" behavior too. (Often times the reason for a failure is in some external dependency or configuration.)

Alternately you could just pin to the previous version of this library!

kaiyoma · 2024-02-13T22:14:27Z

@saperdadsk We run 17.0.2. I'm not sure if we have development mode on ... whatever the default is, I guess?

kaiyoma · 2024-02-13T22:15:17Z

Alternately you could just pin to the previous version of this library!

Yes, that's always our fallback when we run into issues with npm libraries.

saperdadsk · 2024-02-13T22:20:52Z

@kaiyoma In your working version, what does the render function to AutoSizer get called with?

@bvaughn Arguably, this might just mean the change was a behavior change for components that are not attached to the DOM - i.e previously, since https://github.com/bvaughn/react-virtualized-auto-sizer/blob/master/src/AutoSizer.ts#L116C1-L133C1 only checks for the literal 0 for bailing out, NaN values were being allowed and the children function would be called with NaNs - now, the values will actually get set to zero, so that check is being hit.

saperdadsk · 2024-02-13T22:22:16Z

It's ugly to say that Autosizer's contract is "Sometimes the height/width might be NaN and you'll get a warning from react, but your component will still render", but I guess fixing that is technically a breaking change....?

kaiyoma · 2024-02-13T22:23:46Z

@saperdadsk I can't tell, because the render function doesn't seem to get called at all. If I put a console log inside, it never prints out anything to the screen.

saperdadsk · 2024-02-13T22:24:48Z

@kaiyoma In your working version?

bvaughn · 2024-02-13T22:25:00Z

@bvaughn Arguably, this might just mean the change was a behavior change for components that are not attached to the DOM

It's ugly to say that Autosizer's contract is "Sometimes the height/width might be NaN and you'll get a warning from react, but your component will still render", but I guess fixing that is technically a breaking change....?

This seems like a Semver "gray area" topic. I tend to think of it as a bug fix, but maybe it is a breaking change. (To be honest, I don't feel like I fully understanding it yet.)

kaiyoma · 2024-02-13T22:26:10Z

@kaiyoma In your working version?

Ah, sorry. In the working version, I think we get passed NaN, which is the bug I reported here a while back: #69

saperdadsk · 2024-02-13T23:38:33Z

@bvaughn Let me state the previous bug and the fix as I see it:

Consider the following HTML page:

<html>
<head>
<style>
</style>
</head>
<body>
	<div class='wrapper'></div>
	
	<script>
		const wrapper = document.querySelector('.wrapper');
		console.log('Attached', `"${window.getComputedStyle(wrapper).paddingLeft}"`);
		
		const detachedWrapper = document.createElement('div');
		console.log('Detached', `"${window.getComputedStyle(detachedWrapper).paddingLeft}"`);
		
		// Not at all an expert on the spec, but for reference:
		// See https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle , which specifies the getComputedStyle should return an empty https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle if the node is detached, and and https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface end up specifying that that a missing property is returned as an empty string 
	</script>
</body>
</html>

This will log

Attached "0px"
Detached ""

Before my change

If Autosizer was rendered inside wrapper, it would bail out and not render its children (since it determines that the height/width are the value 0).
if Autosizer was rendered inside detachedWrapper, it would not bail out (since it determines the height/width are NaN) and would render children, passing the value NaN as the width and height (which is the issue logged in Is it possible to test AutoSizer-powered components in unit tests? #69)

After my change

If Autosizer was rendered inside either wrapper or detachedWrapper, it would bail out and not render its children

At least in JSDOM, it looks like the handling of detached elements is not consistent with that.

saperdadsk · 2024-02-13T23:39:24Z

Messing around a bit more, I think I can write a test that shows my behavior change:

it("should not render children for elements with empty computed styles", () => {
    mockOffsetSize(0, 0);

    renderHelper({
      excludeWrapperStyling: true,
      height: 0,
      width: 0,
    });

    expect(container.textContent).toEqual("");
  });

(Test will fail with

    Expected: ""
    Received: "width:NaN, height:NaN, foo:456, bar:123"

without my change)

(Not quite a repro of the bug I was encountering, but at least tests the same lines). I can probably open a PR to add that test, if you still think this behavior change is reasonable)

bvaughn · 2024-02-13T23:47:59Z

I think the change made in 28723d1 is both reasonable and a bug fix.

~~I think the test you show above would pass before and after your change though?~~

Edit I just wrote that test locally, and it does pass before and after you commit.

Edit 2 Note that I wasn't accounting for the param you implied, excludeWrapperStyling. Implementing it that causes the test to pass/fail.

bvaughn · 2024-02-13T23:52:39Z

I am getting ready to go on vacation until Monday! I don't really have the mental space to figure this out right now. If you'd like to post a PR or a proposal, I can look at it when I'm back. :) I'm going to step away until then.

This adds a test for the fix for bvaughn#79 - while that situation can appear in more situations than the one tested here (nodes being detached from the DOM, etc), it at least manifests in our test environment if we don't apply any styling to the parent node. Relates to bvaughn#80, so if there is a decision to change the behavior, there is at least a test of _some_ kind

saperdadsk · 2024-02-23T18:33:26Z

Opened a PR with a test case. Thinking on this issue, I'm leaning towards leaving this change in place with no further changes from the lib side. I can't really think of any change that would allow tests to work as before without detrimentally affecting production code (e.g checking for detached node status or something).

bvaughn · 2024-02-24T15:08:34Z

Sorry for the churn and confusion, @kaiyoma.

I'm inclined to leave this change in place as well though. Thanks for digging into this, @saperdadsk.

kaiyoma · 2024-02-24T15:50:20Z

So I lost the discussion a bit here. Are you saying that AutoSizer just won't work inside unit tests anymore?

…

On Sat, Feb 24, 2024, 7:08 AM Brian Vaughn ***@***.***> wrote: Sorry for the churn and confusion, @kaiyoma <https://github.com/kaiyoma>. I'm inclined to leave this change in place as well though. Thanks for digging into this, @saperdadsk <https://github.com/saperdadsk>. — Reply to this email directly, view it on GitHub <#80 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACRZEJAT6Q36ZNDX7NWBYJTYVH677AVCNFSM6AAAAABDHFRGH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSGM4TMOBWGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

bvaughn · 2024-02-24T15:52:15Z

No. It works inside of unit tests. I have unit tests inside this project, and I use autosizer in other projects that have unit tests.

I’m not sure what is going on in your specific test because you didnt share a repro.

kaiyoma · 2024-02-24T17:27:05Z

Okay, I'll try to work on a repro next week.

anambl · 2024-02-28T18:20:45Z

Hi all! I am seeing the exact same issue as @kaiyoma.
I'm trying to understand in @saperdadsk 's comment here, specifically this point

If Autosizer was rendered inside wrapper, it would bail out and not render its children (since it determines that the height/width are the value 0).

How does the padding value relate to the height/width?

I have noticed that if I add a padding on the wrapper where AutoSizer is rendered then it will work, but I don't really understand why that is. In my case the padding is an empty string actually (not entirely sure how as I just simply never provided a padding to it) so for me the change in this PR means that before it used to work with the empty string whereas now it will default to 0.
And before the upgrade, I was also seeing the NaN height/width exactly as described in this issue.

bvaughn · 2024-02-28T18:24:01Z

And before the upgrade, I was also seeing the NaN height/width exactly as described in #69.

Sounds like the update improved things for you then?

anambl · 2024-02-29T09:20:50Z

And before the upgrade, I was also seeing the NaN height/width exactly as described in #69.

Sounds like the update improved things for you then?

I'm not seeing that warning anymore which is great, but now the component doesn't render at all anymore in unit tests so not quite 😅

As I said, it only renders if I add a padding to the element where AutoSizer is rendered. How does the padding value relate to the height/width? Why does no padding assume the height/width are 0?

And just to clarify, I'm only having this problem in unit tests 🤔

bvaughn · 2024-02-29T11:46:58Z

This component does not render children if their height or width is 0px (bc there would be nothing to display, so it just bails out).

In order to calculate the available height and width, it measures the parent’s height and width and then subtracts padding. If the remainder is 0, it bails out.

Previously, the check would cause padding to be parsed as NaN in some cases, which broke the 0 comparison and caused it to not bail out when it should. Now that is fixed.

My guess is that your test is “failing” now bc the auto sizer’s parent component has 0 height or width. It only happened to “work” before because of the NaN bug.

Fix the size of the container auto sizer is in to fix your test.

anambl · 2024-02-29T13:47:06Z

This component does not render children if their height or width is 0px (bc there would be nothing to display, so it just bails out).

My guess is that your test is “failing” now bc the auto sizer’s parent component has 0 height or width. It only happened to “work” before because of the NaN bug.

In the first sentence - by "their" did you mean the parent's size?

I'm not very good at CSS, but I think the parent container's size can be 0 and visually its children could still be visible provided their size is not 0.

Either way, I understand why the fix was good, but I do still think there's something not quite right. Adding a width and height explicitly to the parent of autosizer as an inline style doesn't fix the issue, where as adding a padding does. That doesn't seem right to me. I'm also a bit concerned that it only fails in unit tests, not sure why that is.

For now, I'll pin the version to the one that was working for me, thanks for replying :)

bvaughn · 2024-02-29T14:20:17Z

I'm not very good at CSS, but I think the parent container's size can be 0 and visually its children could still be visible provided their size is not 0.

This component isn't trying to mimic or support all of the CSS spec. It's intended for a specific use case: When you want a React component to render into the full width and/or height of its container. If you want overflow: visible then you want to either use something else, or put auto sizer inside of the component that overflows.

Adding a width and height explicitly to the parent of autosizer as an inline style doesn't fix the issue, where as adding a padding does. That doesn't seem right to me. I'm also a bit concerned that it only fails in unit tests, not sure why that is.

I've asked for a repro above on this issue. I'll ask again now. Share your code and I'll look.

anambl · 2024-02-29T14:42:43Z

This component isn't trying to mimic or support all of the CSS spec.

That's fine, as I said, adding the height and width didn't solve my problem.

I've asked for a repro above on this issue. I'll ask again now. Share your code and I'll look.

I don't have the time right now to put together a small clean example, but the repo I'm working on is public if you want to have a look at that, otherwise I'll try to create a smaller example in the following weeks.

I raised a PR here with the bump: Skyscanner/backpack#3260
AutoSizer is used here: https://github.com/Skyscanner/backpack/blob/main/packages/bpk-component-scrollable-calendar/src/BpkScrollableCalendarGridList.tsx#L165

bvaughn · 2024-02-29T14:47:38Z

I'll likely wait for a smaller repro. That project looks pretty big and I don't have a lot of time to support this package.

bvaughn · 2024-03-01T02:15:47Z

I had a few minutes this evening so I thought I would take a look. Good grief npm install takes a long time on that project.

Checking out your branch, installing dependencies, and running npm run test fails with:

✖ 235 problems (168 errors, 67 warnings)

Next I tried npm run jest– which actually runs tests but shows a lot of failures. Then to compare, I ran the same script on the main branch– and it had the same number of failures:

Test Suites: 99 failed, 190 passed, 289 total
Tests:       20 failed, 857 passed, 877 total

I guess this is why I ask for smaller repros.

anambl · 2024-03-01T09:23:00Z

I had a few minutes this evening so I thought I would take a look. Good grief npm install takes a long time on that project.

Checking out your branch, installing dependencies, and running npm run test fails with:

✖ 235 problems (168 errors, 67 warnings)

Next I tried npm run jest– which actually runs tests but shows a lot of failures. Then to compare, I ran the same script on the main branch– and it had the same number of failures:
Test Suites: 99 failed, 190 passed, 289 total
Tests:       20 failed, 857 passed, 877 total
I guess this is why I ask for smaller repros.

We have a bunch of icons and styles that need to be built first, there's 2 commands you need to run before running the tests.

bvaughn · 2024-03-06T22:51:35Z

Version 1.0.24 adds a new optional prop, doNotBailOutOnEmptyChildren, which can be used to override the default behavior of not rendering children when either width or height are 0

pbrzosko · 2024-03-11T17:28:58Z

@bvaughn @saperdadsk I have analyzed why this issue started to occur. If I am not mistaken, the problem was that before, when paddings were "", height and width were NaNs. Thus the code never went into height === 0 or width === 0 thus it didn't bail out because of them being 0. After that was fixed, it properly bails out.

bvaughn · 2024-03-11T17:38:35Z

Yeah that pretty much aligns with this comment #80 (comment)

pbrzosko · 2024-03-12T07:44:39Z

Right... 🤦 At least that is now more clear than when I read it 😄

bvaughn · 2024-03-12T11:47:34Z

Haha, no worries ☺️

saperdadsk mentioned this issue Feb 23, 2024

Adds a test checking that we handle empty string styles #83

Merged

bvaughn closed this as completed Feb 24, 2024

anambl mentioned this issue Feb 29, 2024

Bump date-fns from 2.30.0 to 3.3.1 Skyscanner/backpack#3186

Merged

Version 1.0.22 is breaking all of our unit tests #80

Version 1.0.22 is breaking all of our unit tests #80

Comments

kaiyoma commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

saperdadsk commented Feb 13, 2024 • edited Loading

saperdadsk commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

saperdadsk commented Feb 13, 2024 • edited Loading

saperdadsk commented Feb 13, 2024

kaiyoma commented Feb 13, 2024 • edited Loading

saperdadsk commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

bvaughn commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

bvaughn commented Feb 13, 2024

saperdadsk commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

bvaughn commented Feb 13, 2024 • edited Loading

kaiyoma commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

saperdadsk commented Feb 13, 2024

saperdadsk commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

saperdadsk commented Feb 13, 2024

bvaughn commented Feb 13, 2024

kaiyoma commented Feb 13, 2024

saperdadsk commented Feb 13, 2024

saperdadsk commented Feb 13, 2024 • edited Loading

bvaughn commented Feb 13, 2024 • edited Loading

bvaughn commented Feb 13, 2024

saperdadsk commented Feb 23, 2024

bvaughn commented Feb 24, 2024

kaiyoma commented Feb 24, 2024 via email

bvaughn commented Feb 24, 2024

kaiyoma commented Feb 24, 2024

anambl commented Feb 28, 2024

bvaughn commented Feb 28, 2024

anambl commented Feb 29, 2024 • edited Loading

bvaughn commented Feb 29, 2024 • edited Loading

anambl commented Feb 29, 2024

bvaughn commented Feb 29, 2024

anambl commented Feb 29, 2024

bvaughn commented Feb 29, 2024

bvaughn commented Mar 1, 2024 • edited Loading

anambl commented Mar 1, 2024

bvaughn commented Mar 6, 2024

pbrzosko commented Mar 11, 2024

bvaughn commented Mar 11, 2024

pbrzosko commented Mar 12, 2024

bvaughn commented Mar 12, 2024

saperdadsk commented Feb 13, 2024 •

edited

Loading

saperdadsk commented Feb 13, 2024 •

edited

Loading

kaiyoma commented Feb 13, 2024 •

edited

Loading

bvaughn commented Feb 13, 2024 •

edited

Loading

saperdadsk commented Feb 13, 2024 •

edited

Loading

bvaughn commented Feb 13, 2024 •

edited

Loading

anambl commented Feb 29, 2024 •

edited

Loading

bvaughn commented Feb 29, 2024 •

edited

Loading

bvaughn commented Mar 1, 2024 •

edited

Loading