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

feat(useAsyncActionHandler): add useAsyncActionHandler hook #1095

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

kseniya57
Copy link
Contributor

No description provided.

@kseniya57 kseniya57 self-assigned this Nov 4, 2023
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@@ -0,0 +1,30 @@
import React from 'react';

export function useAsyncActionHandler<Result>(
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What is Result? There is no such type in the current namespace
  2. It is not convenient to have generic with а required parameter

@@ -0,0 +1,30 @@
import React from 'react';

export function useAsyncActionHandler<Result>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's follow this convention #881

[action],
);

return [isLoading, handleAction];
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change to object here: {isLoading, handler}. Tuples are primary used for [value, setValue]

const handleAction: typeof action = React.useCallback(
(...args) => {
setLoading(true);
return action(...args).finally(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add check if returned value is a promise before calling finally

@kseniya57 kseniya57 force-pushed the async-action-handler-hook branch from 815b4e7 to 6313ddb Compare November 8, 2023 06:09
@kseniya57 kseniya57 requested review from amje and korvin89 November 8, 2023 06:09
@kseniya57 kseniya57 force-pushed the async-action-handler-hook branch from 6313ddb to dc9575e Compare November 9, 2023 08:58
@kseniya57 kseniya57 requested a review from ValeraS November 9, 2023 08:59
@amje
Copy link
Contributor

amje commented Nov 10, 2023

@ValeraS ping

@kseniya57 kseniya57 requested a review from ValeraS November 13, 2023 04:28
@kseniya57 kseniya57 force-pushed the async-action-handler-hook branch from dc9575e to a4ccbfa Compare November 14, 2023 11:48
@kseniya57 kseniya57 requested a review from ValeraS November 14, 2023 11:48
@kseniya57 kseniya57 force-pushed the async-action-handler-hook branch from a4ccbfa to 52de79c Compare November 14, 2023 14:13
@kseniya57 kseniya57 merged commit 66a25b3 into main Nov 14, 2023
3 checks passed
@kseniya57 kseniya57 deleted the async-action-handler-hook branch November 14, 2023 17:30
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.

5 participants