-
Notifications
You must be signed in to change notification settings - Fork 32
fix(console): fix console test errors #1428
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
Conversation
475b0fa to
96d307e
Compare
| // clear the timeout. | ||
| useEffect(() => { | ||
| return () => timerIds.map((timerId) => clearTimeout(timerId)); | ||
| }, [timerIds]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using timerIds as a dependency fine? will it not clear existing timeouts each time you add a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the test shows me that it is working fine, what I am missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. It causes me doubt, I will investigate
src/hooks/useTimeout/index.js
Outdated
|
|
||
| // clear the timeout. | ||
| useEffect(() => { | ||
| return () => timerIds.map((timerId) => clearTimeout(timerId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it is very opinionable, also, that this way is very idiomatic for some JS people.
I am not asking for a change, only starting a conversation.
In my opinion, it is better to keep functions like map to map, it means to convert each item of a list into a different item in a new list without side effects.
clearTimeout is a effect, for that reason, I think that for will be clear here.
I know that it is not for JS ecosystem, but I think that these Eric Lippert's reflections applyies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't seen it that way, but it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it gives more expression to the code
src/hooks/useTimeout/index.test.js
Outdated
| useEffect(() => { | ||
| createTimeout(callback, delay); | ||
| createTimeout(callback2, delay2); | ||
| }, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useEffect is not necessary here right?
| useEffect(() => { | |
| createTimeout(callback, delay); | |
| createTimeout(callback2, delay2); | |
| }, []); | |
| createTimeout(callback, delay); | |
| createTimeout(callback2, delay2); |
I mean, useTimeout already has the useEffect inside, and in our real scenario, we are not using useTimeout inside useEffect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the useEffect in renderHook's to be able to execute the createTimemout function. it is the easiest way to use it. in the scenario we have right now (form-helpers) it is not used in the useEffect, but it could be used, since it is a function
If I don't put it, it will show me that the setTimeout has been called too many times.
what I can do is simulate that these calls to createTimeout are made within a function, and trigger the function by clicking on a fake button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
|
Nice! 👍 |
6bd8ba9 to
28eba8a
Compare
|
🎉 This PR is included in version 1.179.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |

Fix console test errors
Add useTimeout hook in form-helpers and clean errors in console
useTimeout usage mode
const createTimeout = useTimeout();createTimeout(callback, delay)escenario:
Results:
Local

Jenkins
