Skip to content
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

Check if requestAnimationFrame is defined in shouldUpdate #81

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

branberry
Copy link
Contributor

Fixes test issues with Vitest in Recharts

@@ -10,7 +10,7 @@ export default function setRafTimeout(callback, timeout = 0) {
callback(now);
currTime = -1;
} else {
requestAnimationFrame(shouldUpdate);
if (typeof requestAnimationFrame !== 'undefined') requestAnimationFrame(shouldUpdate);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing typeof requestAnimationFrame !== 'undefined' for the conditional because using !requestAnimationFrame as the conditional still caused the undefined error. This ensures that we check without throwing an error.

Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although can't you just check for a falsy requestAnminationFrame instead of type of?

@branberry
Copy link
Contributor Author

branberry commented Sep 26, 2023

Hey @ckifer ! Do you mean something like !!requestAnimationFrame? I haven't tried that, but when I was debugging this code in the Vitest tests, doing the following still gave me the requestAnimationFrame is undefined error:

 if (!requestAnimationFrame) { ... }

Typically wouldn't do the whole typeof stuff, but for whatever reason, this was the only way to ensure that it wouldn't error out when checking.

@ckifer
Copy link
Member

ckifer commented Sep 26, 2023

Weird yeah the second way is what I was thinking should work? Strange... generally I would think libraries shouldn't have to adjust for a test framework but.. makes sense I guess?

@branberry
Copy link
Contributor Author

Yeah.. It was really bizarre. I think it's all related to VItest interacting with the react-testing-library which leads to fake timers not working: testing-library/react-testing-library#1198

When making the tests async, stuff just didn't want to behave nicely for the animated tests, and the library would still be calling requestAnimationFrame after the tests have completed, so I think this is why we ran into this. I also debugged into some gnarly Vitest code, and it looked like that was the case.

@ckifer
Copy link
Member

ckifer commented Sep 26, 2023

JsDom in general is probably a lot of the issue because everything animation and actual dom testing wise needs to be mocked

@ckifer ckifer merged commit eecb6b0 into recharts:master Oct 10, 2023
1 check passed
ckifer pushed a commit to recharts/recharts that referenced this pull request Oct 17, 2023
Updated tests to pass using Vitest. This PR depends on a [change in the
react-smooth](recharts/react-smooth#81) library
to deal with the unhandled exceptions we receive in the tests.

vitest-dev/vitest#3117

Additional changes:
- updated the test command to not watch by default. This is to prevent
pushes from being stuck due to running the test in watch mode for the
push husky checks.
- turned off the no-extraneous-dependencies error so that we no longer
receive errors when importing `vitest`.
- have Vitest ignore `node_modules` and `react-smooth` tests.

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Nikolas Rieble <[email protected]>
Co-authored-by: “Nikolas <“[email protected]“>
Co-authored-by: Pavel Vaněček <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hong Seungwoo <[email protected]>
Co-authored-by: Coltin Kifer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants