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

New hook: useSafeTimeout #19109

Closed
wants to merge 2 commits into from
Closed

New hook: useSafeTimeout #19109

wants to merge 2 commits into from

Conversation

ellatrix
Copy link
Member

Description

Moves the logic within withSafeTimeout to the new hook useSafeTimeout.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

export default function useSafeTimeout() {
const timeouts = useRef( [] );

function clearTimeout( id ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we use useCallback for these?

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Dec 12, 2019
@ellatrix ellatrix changed the title New hook: newSafeTimeout New hook: useSafeTimeout Dec 12, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

To be fair, I'm not sure whether we really need this hook. I feel this is just a Higher-order that was needed because of how class components work. I feel with hooks, we don't really need it.

@ellatrix
Copy link
Member Author

@youknowriad Could you elaborate? Why won't we need it?

@youknowriad
Copy link
Contributor

I feel when you use timeouts with hooks, you'll always do something like that;

useEffect(() =>  {
  const tid = setTimeout(dosomething);
  return () => clearTimeout(tid);
});

The timeout and its clearance are located in the same code, the chance to forget about this and the added code here is very small while in the case of a class Component or components without hooks, it's quite complex to achieve: you need refactor to a class component and use separate methods to trigger and clear the timeouts. All these are fixed with hooks.

@ellatrix
Copy link
Member Author

Ah, you're right. I was thinking of a use case in RichText where setTimeout wouldn't be in useEffect, but this seems very rare.

@aduth
Copy link
Member

aduth commented Jan 27, 2020

Ah, you're right. I was thinking of a use case in RichText where setTimeout wouldn't be in useEffect, but this seems very rare.

There's now another example of this (timers in event handler) at #19881, though I'd agree that it's still not yet (to me) at a point to need a higher-level solution, and if anything its absence could serve as a deterrent to an anti-pattern.

torounit added a commit that referenced this pull request May 21, 2022
torounit added a commit that referenced this pull request Jun 2, 2022
torounit added a commit that referenced this pull request Jun 2, 2022
…41216)

* refactor `<TokenInput>` to functional component and Typescript.

* Remove findRenderedComponentWithType as it does not work with functional components.

* use findRenderedDOMComponentWithTag instead of findRenderedComponentWithType

* convert `<Token>` to TypeScript

* type only import

* convert SuggestionsList to TypeScript

* Change to not use withSafeTimeout. ref: #19109

* Remove comment because tabIndex is not specified.

* add type for dom-scroll-into-view

* add form-token-field dir to include in tsconfig.json

* refactoring types.

* fix type.

* refactor FormTokenField to TypeScript and Functional Component

* refactor story

* allow set suggestions in stories.

* Use act where useEffect is relevant.

* separate state

* revert ts-nocheck

* remove onChange prop form storybook

* Update packages/components/src/form-token-field/token-input.tsx

Co-authored-by: Lena Morita <[email protected]>

* use ComponentProps

* add type docs

* replace token-field-wrapper to tsx

* add doc comment for FormTokenField

* fix type docs

* Update packages/components/src/form-token-field/types.ts

Co-authored-by: Lena Morita <[email protected]>

* Update packages/components/src/form-token-field/types.ts

Co-authored-by: Lena Morita <[email protected]>

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <[email protected]>

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <[email protected]>

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <[email protected]>

* change variable and function name

* Fix useEffect to updateSuggestions().

* Disabled control for __experimentalValidateInput. Update type document.

* add changelog

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <[email protected]>

Co-authored-by: Lena Morita <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants