Skip to content

feat(notification): Add overload support for notify function to enhance flexibility #289

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sanduluca
Copy link
Contributor

This PR introduces overload support for the notify function, enabling it to handle multiple usage patterns in a type-safe manner. The changes improve developer ergonomics by allowing the function to be called in various ways:

Supported Call Patterns

notify('error', title);
notify('error', title, description);
notify('error', title, setup);
notify('error', title, description, setup);
notify('error', setup);

Key Changes

  1. Function Overloads: Added multiple overloads to the Notify type to define various valid function signatures.
  2. Flexible Implementation: Updated the implementation of notify to dynamically handle different combinations of title, description, and setup arguments using type guards.
  3. Type Safety: Ensured the notify function adheres to TypeScript's type safety, allowing clear and precise error messages during development.

Motivation

The notify function previously required a rigid setup object for all calls, which reduced flexibility. These enhancements allow developers to quickly show notifications with minimal boilerplate while retaining the ability to configure advanced notification options.

Examples

notify('error', 'Something went wrong');
notify('error', 'Something went wrong', 'Additional details about the error');
notify('error', 'Something went wrong', {
  params: { title: 'Something went wrong', description: 'Details' },
  config: { timeout: 5000 },
});
notify('error', 'Something went wrong', 'Details', {
  params: { title: 'Something went wrong', description: 'Details' },
  config: { timeout: 5000 },
});
notify('error', {
  params: { title: 'Something went wrong', description: 'Details' },
  config: { timeout: 5000 },
});

Let me know if any additional information or refinements are needed!

Added overloads to the `notify` function to support flexible usage patterns, allowing calls with title, description, setup, or combinations of these. Updated the implementation to handle different argument structures dynamically.
@sanduluca sanduluca changed the title Add overload support for notify function to enhance flexibility feat(notification): Add overload support for notify function to enhance flexibility Jan 15, 2025
@PdoubleU
Copy link
Contributor

Hi @sanduluca thanks for such improvement, I like TS's ability to implement a function overload mechanism. Give me a couple of days, I'll take a look and see if we can release it.

@sanduluca
Copy link
Contributor Author

up

Comment on lines 19 to 22
notificationType: any,
titleOrSetup: any,
descriptionOrSetup?: any,
setup?: any
Copy link
Contributor

@PdoubleU PdoubleU Mar 21, 2025

Choose a reason for hiding this comment

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

Is there a better option than 'any' here? Maybe doesn't type as 'any' but use only the inferred types from the Notify

@PdoubleU
Copy link
Contributor

In general good PR, I'd like to test it first to check if it has fully backward compatibility with the current interface. Then deploy :)

The main issue here is that Notify is a function type with multiple overloads, meaning it can accept different sets of parameters. TypeScript resolves function overloads at the call site, not within the function implementation. This makes it difficult to infer a generic tuple type for the parameters in a way that works across all overloads.

Workarounds
- Explicit Typing
- Explicit Overload Handling
- Conditional Type Checking
@sanduluca
Copy link
Contributor Author

kind reminder

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.

2 participants