add type checking to asyncAct args #469
Conversation
|
Thanks. What happens if console.error is called with a non-string? I believe that currently this will result in the originalConsoleError not getting called. |
|
That is correct. I can move the else out of the if block to have it run whenever there is no string. |
src/__tests__/old-act.js
Outdated
| test('async act handles values that are not strings', async () => { | ||
| try { | ||
| await asyncAct(() => { | ||
| throw new Error({error: 'test error'}) |
There was a problem hiding this comment.
I think you have to do a console.error({ message: 'some message' } insteadof throwing it, to mimic the problem
There was a problem hiding this comment.
Changed the test to not throw error.
There was a problem hiding this comment.
This is correct 👍
but you had to change the assertion of another test, which is an indication of a wrong implementation (you are still not calling the original console.error)
check this implementation for inpiration :-):
rvdkooy@9020b83
@kentcdodds can you have a look if we're on the right track with this?
| @@ -32,18 +32,15 @@ test('async act works even when the act is an old one', async () => { | |||
| console.error('sigil') | |||
| }) | |||
| expect(console.error.mock.calls).toMatchInlineSnapshot(` | |||
| Array [ | |||
| Array [ | |||
| "sigil", | |||
There was a problem hiding this comment.
It looks like this log is no longer happening. Could you fix your changes so you don't have to change this snapshot?
There was a problem hiding this comment.
Changed implementation to revert back to this snapshot.
| ) === 0 | ||
| ) { | ||
| // no-op | ||
| } |
There was a problem hiding this comment.
| } | |
| } else { | |
| originalConsoleError.apply(console, args) | |
| } |
I think that should do it.
|
It looks like this is still unresolved... That snapshot should not change at all. |
|
@ely-alamillo check this PR for the solution: |
|
Thank you @rvdkooy. I've merged your PR. Thank you @ely-alamillo for doing the initial work here. Even though we weren't able to merge your PR, I'm still happy for your efforts in contributing to the project! |
|
@all-contributors please add @ely-alamillo for code and tests |
|
I've put up a pull request to add @ely-alamillo! 🎉 |
What:
This PR fixes issue: #442 by adding type checking for args used in asyncAct function.
Why:
Current implementation assumes console.erorr was always called with a string even though it could be called with other types.
How:
Wrapped execution of code in a type check if statement to ensure operations are done only on string types.
Checklist:
docs site N/A