smastrom / notivue

🔔 Powerful toast notification system for Vue and Nuxt.
https://notivue.smastrom.io
MIT License
633 stars 7 forks source link

Add push inside a composable #58

Closed philippedasilva-orizone closed 1 month ago

philippedasilva-orizone commented 1 month ago

Hi @smastrom

First of all, great library you came up with: it's clean, clear and easy to use.

This issue isn't a mandatory one and, to be honest, I don't even know if it is a good request or not performance wise. However, using a global push to add or manage notifications isn't on my best practice as it requires the developers to know that push is actually coming from Notivue especially on large code base. I'd rather use something like:

const { push } = useNotivue()
// or even
const notivue = useNotivue()
notivue.info({...})
notivue.promise({...})

because it tells the developer reading the code where it actually comes from. Once again not necessarily a huge requirement but it goes along what I've experienced so far using Vue or Nuxt on their version 3.

For this purpose, I'm actually wrapping it inside my own composable and it works just fine but I thought I should share that thought with you.

Happy to implement it myself and make a pull request if you think it deserves some attention ;)

smastrom commented 1 month ago

Hi @philippedasilva-orizone, I totally get what you mean but there isn't much we can do about that. Let me explain you why.

It is obvious that on large Nuxt codebases, people would have to guess or inspect where push comes from which is not ideal. This happens because Nuxt 3 goes all-in with auto-imports not because Notivue could have done a better job at naming functions or at creating wrappers.

Nuxt modules consumers expect to be able to auto-import any function upon installation so this is something that must be provided and enabled by default.

Then, given that I still think the overall API nomenclature is great:

import { push } from 'notivue'

push.success('Hello!')
push.info('Hi!')

If auto-importing (or importing) push turns out to be counterproductive, it takes 30 seconds to create a tiny wrapper on top of it (as you already did) or for example, to re-export { push as notify } from 'notivue'. That's it.

You can really do whatever you want with notivue's primitives, there are virtually no restrictions.

What I can probably do in a future release is to add an option to the Nuxt module config to disable auto-imports in order to avoid clashes and potentially improve customization but more than that there's nothing I can or should do.

To conclude, including push in a composable or composable-like function (function that starts with use*) would be a mistake. By convention, Vue composables are functions meant to be called only in the setup context (because they are not supposed to work outside of it) while push can instead be called from anywhere in your app JS code.

That said thank you for your interest and for your initial kind words, I hope you'll keep enjoying Notivue! Cheers!

philippedasilva-orizone commented 1 month ago

Makes total sense @smastrom

Thanks for the clear and lengthy response. Keep up this great work ;)