-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
refactor(hooks): refactor hooks #4253
Conversation
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.
LGTM. But I would suggest break large pull requests into separate small changes.
Note this pull requests consists of several different refactors that are readily to be broken up.
if (!isServer) { | ||
on(document, 'keydown', closeModal) | ||
} | ||
if (isClient) useEventListener(document, 'keydown', closeModal) |
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.
This is not side-effect free. Shall we use some init function instead?
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.
Are there any side effects of useEventListener? I didn't get your point.
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.
@HerringtonDarkholme Can you elaborate what the side effect is here?
@HerringtonDarkholme Each refactoring corresponds to a commit. I think rebase merge is acceptable. There should be no need for one commit for one pull request. |
packages/hooks/use-timeout/index.ts
Outdated
const registerTimeout = (fn: (...args: any[]) => unknown, delay: number) => { | ||
cancelTimeout() | ||
;({ stop } = useTimeoutFn(fn, delay)) | ||
} | ||
const cancelTimeout = () => stop?.() |
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 the usage here is wrong, please refer to the source code to get appropriate invoking method.
f6d1054
to
27d5959
Compare
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.
LGTM, let's wait for @HerringtonDarkholme's input.
Please make sure these boxes are checked before submitting your PR, thank you!
dev
branch.