Omit certain types of toasts about react-hot-toast HOT 10 OPEN

I'd be happy to make a PR for this -- I've looked at the code, and although it's not a complicated change, I'm torn about how to approach it. @timolins if you want to give me some feedback about which approach is preferred from your perspective (and more likely to be approved/merged), I'll go ahead and open a PR.

Approach 1: omit undefined messages

Right now, the type definitions require that in a call to toast.promise(msgs), all 3 toast-type properties (loading, success, and error) must be present in msgs, and can be null but not undefined. So, it seems reasonable to make a change so that if the message value for a given toast type is undefined (or is a function that returns an undefined value), that toast type would be omitted from display.

For instance, an invocation might look like: toast.promise({success: 'Yay!', error: 'Oops!'}).

My only hesitation here is that some users may not be using TypeScript types, or might be (knowingly or not) passing in undefined values anyway. In this case, currently they would get the same result as passing a message value of null: an appropriately-styled toast, including the icon, but no text. So although technically undefined is not currently an allowed value for a message, in some real-world edge cases this could be a breaking change.

Approach 2: add an option for disabling certain toast types

An option could be added (e.g. disabled) which can cause a certain toast type to be omitted when called from a toast.promise().

For instance, an invocation might look like: toast.promise({success: 'Yay!', error: 'Oops!'}, {loading: {disabled: true}}).

As part of this change, it would be necessary to change the type definitions so undefined is explicitly an allowed message value. This usage is also more verbose and less convenient than the first approach. However it more thoroughly guards against breaking existing edge-case behavior.

from react-hot-toast.

Have a usecase for omitting the "success" message, as well:

• user signs a transaction (pending toast, potential error message but no success message),
• we wait for the transaction to happen (pending toast, success and error messages possible).

It's just awkward to confirm (with a success message) that the user has just confirmed (with a signature).

from react-hot-toast.

@numpde Ah I assumed by "no success message" you meant just the toast with no text. My bad.

Looking numpde's example, indeed it could make sense to just omit the whole toast when nothing is passed (undefined), making the properties passed to toast.promise optional, with a minimum of one.

The biggest worry I have is signature for more complex cases. Let's maybe look at them side by side.

This is what the signature would become between the two, these are all the cases I could come up with:

Simple case (no difference)

// undefined
const result = await toast.promise(myFn, { success: 'worked', /* rest */ })

// null
const result = await toast.promise(myFn, { success: 'worked', /* rest */ })

Inlined condition (more idiomatic with null)

// undefined
const result = await toast.promise(myFn, { success: () => { if (condition) return 'worked' }, /* rest */ })

// null
const result = await toast.promise(myFn, { success: condition ? 'worked' : null, /* rest */ })

Passing as variable based on condition (no major difference, slightly more flexibility with null)

// undefined
let success;
if (condition) success = 'worked'
const result = await toast.promise(myFn, { success, /* rest */ })

// null
const success = condition ? 'worked' : null
const result = await toast.promise(myFn, { success, /* rest */ })

Always omitting success (explicit vs not explicit)

// undefined
const result = await toast.promise(myFn, { 
  loading: 'loading message',
  error: (err) => `error message: ${err}`,
})

// null
const result = await toast.promise(myFn, { 
  loading: 'loading message',
  success: null,
  error: (err) => `error message: ${err}`,
})

Omitting error with predefined other messages (again explicit vs not explicit)

// undefined
const success = 'worked'
const loading = 'loading'
const result = await toast.promise(myFn, { loading, success })

// null
const success = 'worked'
const loading = 'loading'
const result = await toast.promise(myFn, { loading, success, error: null })

Personally I'd go with null because of its slightly cleaner and more explicit signature, but I guess we could maybe vote?

🚀 = I prefer undefined signature
❤️ = I prefer null signature

from react-hot-toast.

Love this library! And just bumped into a use case for this, where I'd only want to show something in case of an error, when defining the toast for a promise (and silently load and succeed otherwise).

This still looks like it it would be an helpful feature.

I'd suggest approach 1 from @zacronos, except undefined should never be explicitly set by users. Secondly we don't want users to accidentally forget to define success, error or loading states.

Therefore I think using explicit null for omitting the toast type would be most appropriate.

from react-hot-toast.

Therefore I think using explicit null for omitting the toast type would be most appropriate.

The drawback to that approach is that null is already an allowed value, with a very specific result: an appropriately-styled toast, including the icon, but no text. This is no doubt a value currently in use by many applications.

Changing the result of passing in an explicit null from that to omitting the toast altogether would be a breaking change for those applications, and so would require a major version bump. So, although I would agree with you if this were a discussion about a new library without an existing user base, for practical reasons I don't think changing the intent of explicit null in this way would work.

from react-hot-toast.

To be honest, what's the point of having an empty toast message? Do we really expect many people to use it?

Either way, we could keep that behaviour by passing an empty string instead of null. I believe it would be more semantically correct (no point to render an element for the text if there is an empty string anyway) and worth the major bump.

The upgrade steps would simply be "replacing null with ''". We could even automate the process if you indeed expect many people to be affected.

from react-hot-toast.

@numpde good to know! Would replacing null with '' work in your case?

from react-hot-toast.

@numpde good to know! Would replacing null with '' work in your case?

Not sure I understand. I'd expect this kind of construct

const tx = await toast.promise(
    contractCall,
    {
        loading: "Waiting for signature...",
        error: "Signature rejected.",
    }
);

to not show anything on success. It seems logical that an empty string should correspond to an empty toast (although it currently looks a bit asymmetrical). I have no opinion on null.

from react-hot-toast.

No offense intended @webbertakken, but you are expressing some opinions as facts.

There's no reason undefined shouldn't be explicit. If you're willing to consider that it is acceptable for a user to explicitly pass undefined for the sake of being explicit, then my suggested approach 1) can use the same signature format that you're advocating for with null. For instance,

const result = await toast.promise(myFn, { success: condition ? 'worked' : undefined, /* rest */ })

Explicit is not always cleaner. If someone wants to show a toast only on error (for something that should otherwise be invisible to the user), I think this:

const result = await toast.promise(myFn, { error: 'Something bad happened!' })

is cleaner than:

const result = await toast.promise(myFn, { loading: null, success: null, error: 'Something bad happened!' })

In the end, "clean" is often an opinion. But consider this more extreme example: when passing a config object, if there are 30 possible options, would you suggest it is cleaner to pass null explicitly for 29 options, in order to indicate the default value should be used for all except the 1 option you want to set?

Using undefined explicitly to indicate lack of toast has the advantage of more explicit code as you want, but also allows the simpler and cleaner omission syntax, and most importantly this represents an extension to the existing typescript signatures rather than a backward-incompatible change to them -- and so is possible without a major version bump.

I actually do agree with you that, if this were a new or v0.x project, it would be worth changing to use '' for lack of message and null for lack of toast (though I still think there's value in allowing omission as well). But, in my experience, frequent version bumps are painful enough for users that it is worth avoiding them when possible. It all boils down to what the project maintainer thinks and wants to prioritize -- if they even think this feature is worth supporting.

from react-hot-toast.

Thanks for your input @zacronos. Please note that the examples and the poll are merely intended to move the discussion forward (and potentially create a PR) to help make the maintainers life a bit easier.

Explicit is not always cleaner. If someone wants to show a toast only on error (for something that should otherwise be invisible to the user), I think this:

const result = await toast.promise(myFn, { error: 'Something bad happened!' })

is cleaner than:

const result = await toast.promise(myFn, { loading: null, success: null, error: 'Something bad happened!' })

Fair point! Personally I have no preference between the two for this specific signature, in this case where the options object of the programmatic api only has 3 potential properties. I'd say, both have pros and cons.

About expressing opinions as facts:

There's no reason undefined shouldn't be explicit.

While this is technically true, semantically it makes a real difference. null adds information about the implementers intention. Using the difference correctly makes for better quality software. Let me try to explain:

Consider wanting to pass an object that conforms to Partial<Options>.

By omission, it simply works:

const options = { loading: 'loading' } 
console.log(Object.hasOwnProperty('success')) // false

By explicitly passing undefined we see behaviour that not every developer realises:

const options = { loading: 'loading', success: undefined }
console.log(Object.hasOwnProperty('success')) // true

This also means the type Option will have to account for string | null | undefined or Options has to use Option | undefined. That doesn't yet include the function signature. The combination of everything trickles down to a fair bit of additional code when done correctly. I've also seen the key: undefined case left unnoticed and unhandled quite a few times by newer developers.

Of course you can replace null types with undefined and implement undefined checks instead of null checks, but in that case you lose information. This might be fine in many cases but it's also a slippery slope. Therefor I would regard manually passing undefined generally as bad practice.

Or as Dan Abramov explains in his excellent course Just Javascript:

In practice, null is used for intentionally missing values. Why have both null and undefined? This could help you distinguish a coding mistake (which might result in undefined) from valid missing data (which you might express as null). However, this is only a convention, and JavaScript doesn’t enforce this usage.

I agree it's ultimately up to the maintainer to decide. And I also think that the vote is useful to give an indication from the community as more brains catch more cases. Your vote would be greatly appreciated as well.

from react-hot-toast.