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

[BUG] Success and Error notifications cannot return undefined to fallback to default #6270

Closed
PercivalFigaro opened this issue Aug 19, 2024 · 6 comments · Fixed by #6327, #6336 or #6404
Closed
Labels
bug Something isn't working good first issue Good for newcomers up for grabs

Comments

@PercivalFigaro
Copy link

PercivalFigaro commented Aug 19, 2024

Update

This issue was initially opened up as a feature request and converted to a bug. The requested feature is possible with the current implementation but the types are not supporting returning undefined from successNotification and errorNotification props of the hooks when passed as functions.

Check out the comment #6270 (comment) about the issue and the fix.

Is your feature request related to a problem? Please describe.

If there are field validation errors from the server side, I want to hide the snackbar (by using a custom errorNotification and then simply not displaying it), but otherwise, display the default error notification snackbar. To do that, I currently need to recreate the default error manually (unless I'm mistaken).

useForm<Translation, HttpError>({
    refineCoreProps: {
      errorNotification: error => {
        if (error?.response?.data?.fieldError) return false

        return {
          message: `There was an error creating your resource (status code: ${error?.statusCode})`,
          description: error?.message,
          type: 'error',
        }
      },
    },
  })

Describe alternatives you've considered

There should be a way to 'passthrough' and use the default notification if my condition to use a custom errorNotification is not met

Additional context

No response

Describe the thing to improve

Instead of the code above, it should be possible to return the original error notification, like so:

useForm<Translation, HttpError>({
    refineCoreProps: {
      errorNotification: (error, originalErrorNotification) => {
        if (error?.response?.data?.fieldError) return false

        return originalErrorNotification
      },
    },
  })
@PercivalFigaro PercivalFigaro added the enhancement New feature or request label Aug 19, 2024
@BatuhanW
Copy link
Member

Hey @PercivalFigaro can you try returning true as a fallback?

@aliemir
Copy link
Member

aliemir commented Aug 20, 2024

Looks like there's an issue with the types here. When passing successNotification or errorNotification props as functions, the type allows returning OpenNotificationParams or false but actually you can also pass undefined to fallback to the default notification config.

You can check if this works for you by adding @ts-expect-error or similar. If this works for you, we can convert this issue to a bug report and resolve it by updating the related types 🤔

@PercivalFigaro
Copy link
Author

@aliemir Yeah, returning undefined works nicely, but generates type errors not only in the notification related part of code, but also in another line in my project:

const translation = query?.data?.data as Translation

I can work around with @ts-expect-error, but I'd appreciate if this was made into a bug report, if this is also fine with @BatuhanW. Thanks :)

@aliemir
Copy link
Member

aliemir commented Aug 21, 2024

@PercivalFigaro I've also noticed that when we use @ts-ignore or @ts-expect-error directives in useForm hook props, it breaks the generics and this also breaks the return type (for this case query), when we implement the proper types this will work as expected.

The fix should be simple as updating the function type of successNotification and errorNotification to include undefined in the return type.

Here are the related lines:

successNotification?:
| OpenNotificationParams
| false
| ((
data?: TData,
values?: TVariables,
resource?: string,
) => OpenNotificationParams | false);
/**
* Error notification configuration to be displayed when the mutation fails.
* @default '"There was an error creating resource (status code: `statusCode`)" or "Error when updating resource (status code: statusCode)"'
*/
errorNotification?:
| OpenNotificationParams
| false
| ((
error?: TError,
values?: TVariables,
resource?: string,
) => OpenNotificationParams | false);

I'll update the issue title and description as bug. Please let us know if you can work on this one, we'll be happy to see your contribution 🚀 🚀

@aliemir aliemir added bug Something isn't working good first issue Good for newcomers and removed enhancement New feature or request labels Aug 21, 2024
@aliemir aliemir changed the title [FEAT] Add a way to fall back to default errorNotification/successNotification [BUG] Success and Error notifications cannot return undefined to fallback to default Aug 21, 2024
@aliemir aliemir added this to the September Release milestone Aug 21, 2024
@emrecancorapci
Copy link
Contributor

emrecancorapci commented Sep 1, 2024

@aliemir I would like to be assigned to this issue if it's still available.

@BatuhanW
Copy link
Member

BatuhanW commented Sep 5, 2024

Hey @emrecancorapci, assigned issue to you.

@BatuhanW BatuhanW added this to the October Release milestone Sep 5, 2024
aliemir added a commit that referenced this issue Sep 16, 2024
@BatuhanW BatuhanW linked a pull request Oct 14, 2024 that will close this issue
@aliemir aliemir linked a pull request Oct 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers up for grabs
Projects
None yet
4 participants