becem-gharbi / nuxt-naiveui

Unofficial Naive UI module for Nuxt
https://nuxt-naiveui.bg.tn
MIT License
163 stars 20 forks source link

Feature Request: Provide a `useNotification()` abstraction that works outside of script-setup #36

Closed BracketJohn closed 10 months ago

BracketJohn commented 10 months ago

Problem / TLDR

useNotification() cannot be used outside of script setup. Using createDiscreteApi as the proposed alternate has multiple downsides (configuration, bundle-size).

It would be useful to have a better useNotification() abstraction that also works outside of script setup, e.g., inside (Nuxt 3) plugins.

Description

We have a Nuxt 3 app with a tanstack/query-plugin. We want to trigger notifications from there when queries fail (as is recommended by them: https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose#defining-on-demand-messages).

We cannot use useNotification() inside the plugin as naive-ui does not allow usage of useNotification() outside of script-setup. For this they recommend to use createDiscreteApi (see https://www.naiveui.com/en-US/os-theme/components/discrete). As far as I can tell useNotification() does not work outside of script-setup as it uses the vue-inject pattern.

Based on this our solution so far is to use useNotification() inside our script-setup and createDiscreteApi inside the plugin. This approach has multiple problems:

  1. bundle size: using createDiscreteApi inlines message, notification, dialog and modal into the entry-point-bundle (see screenshot 1 below)
  2. configuration is not shared between useNotification and createDiscreteApi
  3. naive-ui has an ominous warning inside their docs about not using both in the same app
  4. to work createDiscreteApi created a new, separate vue-app (see their code on this), which cannot be good

(2) can be resolved by sharing config between the n-notification-provider and createDiscreteApi, e.g., via app.config.ts. The other points cannot be improved upon as far as we could tell.

I see two possible solutions:

Would it be possible for this module to provide a better useNotification() abstraction that works both in script setup and outside of it?

And if this is not possible: Would it be possible to at least tree-shake createDiscreteApi?

Assets

screenshot 1: naive-ui entrypoint bundle

image
becem-gharbi commented 10 months ago

In my opinion, the first solution is not feasible as useNotification should be called from inside n-notification-provider in order to inject the api. Regarding the second solution, there is a single export (createDiscreteApp) so I see that tree-shaking is not an option.

The solution I see is to create a child of n-notification-provider and trigger the notification on a custom event.

<!-- app.vue -->

<template>
  <client-only>
    <n-notification-provider>
      <Notification/>
    </n-notification-provider>
   </client-only>
</template>
<!-- Notification -->

<script setup>
const notification = useNotification();

window.addEventListener("naive:notification", ({ detail }) =>
  notification.create(detail)
);
</script>
// plugin.client.ts

import type { NotificationOptions } from "naive-ui";

export default defineNuxtPlugin(() => {
 //
  window.dispatchEvent(
    new CustomEvent<NotificationOptions>("naive:notification", {
      detail: {
        type: "error",
        title: "notification",
        content: "hello",
      },
    })
  );
//
});
BracketJohn commented 10 months ago

Thanks for getting back so quickly!

One thing I dislike about your example is that it wraps the whole app in client-only-tags:

Regarding the latter I think that this (without the client-only) would be a really helpful feature for the module, as it (a) is quite intricate to setup correctly by the module-user and (b) is quite a hard error to debug & find this issue as a workaround for.

Other than that this could be a really smart way to handle it. Doing this you could even create a composable that can then be used in both cases, right? This could be done by as follows

  1. module takes care of registering the event listener (and maybe even adding the notification provider)
  2. module shares a "clever" composables, that tries to use useNotification() and falls back to emitting an event instead if on the client-side

Rough sketch for clever composable:

// file: ~/composables/useCleverNotification.ts

// this could probably be done smarter / better
const useNotificationFallback = () => ({
  success: ({ detail }) => new CustomEvent<NotificationOptions>("naive:notification", {
      detail: {
        type: "success",
        ...detail
      },
    })
  }
})

export default () => {
  let notification; 
  try {
    notification = useNotification()
  } catch (error) {
    if (!process.server) {
      notification = useNotificationFallback()
    }
  }

  return notification
}
becem-gharbi commented 10 months ago

Edit: I will think more into this. Thanks for this issue, appreciate it!

BracketJohn commented 10 months ago

Aha, I didn't get your example but now I do, thanks!

I tested this out on our app now and it works like a charm. What was a bit misleading to me about your example above was that you have a client-only tag around your whole app.vue.

I structure it like this now to make this nicely usable around our app in places that cannot use useNotification() directly. I'm documenting it here so that it may help other people & maybe you can point out some other ideas of improving this (or come up with a useful, module-fitting abstraction you want to add).

So without further ado, here's the step-by-step manual to make this work (nicely):

  1. Create a dispatch.ts utility to (a) reduce boilerplate of window.dispatchEvent(...) and (b) have a single place that specifies the custom eventName to be used

    // file: ~/components/TheEventBasedNotification/dispatch.ts
    import type { NotificationOptions } from 'naive-ui'
    
    export const eventName = 'naiveui:notification'
    export default (detail: NotificationOptions) => window.dispatchEvent(new CustomEvent<NotificationOptions>(eventName, {
      detail,
    }))
  2. Create the listener client-only component Listener.client.vue (notice the .client.vue) that listens for eventName

    // file: ~/components/TheEventBasedNotification/Listener.client.vue
    <script setup lang="ts">
    /**
     * This is a workaround for a naive-ui limitation: `useNotification` only works within `script setup` of the app. So we cannot
     * use it inside nuxt-plugins, such as our [vue-query](~/plugins/vue-query.ts) plugin.
     *
     * This workaround works by:
     * 1. registering an event listener here
     * 2. dispatching events using the `./dispatch.ts` method in places that cannot use `useNotification()` directly
     * 3. if this component already mounted it will catch the event and create a notification based on it
     *
     * This workaround was proposed & discussed with the maintainer of `nuxt-naiveui` here: https://github.com/becem-gharbi/nuxt-naiveui/issues/36#issuecomment-1846970417
     */
    import type { NotificationOptions } from 'naive-ui'
    import { eventName } from './dispatch'
    
    const notification = useNotification()
    
    // @ts-expect-error This is a custom handler that does not match any default event-handler-types
    window.addEventListener(
      eventName,
      ({ detail }: { detail: NotificationOptions }) => notification.create(detail)
    )
    </script>
    
  3. Add the Listener.client.vue inside your app.vue, as a child to the n-notification-provider
    // file: ~/app.vue
    <template>
      <NNotificationProvider v-bind="notificationConfig">
        <!-- Add the event based listener as a child of the naive-ui notification provider here: -->
        <TheEventBasedNotificationListener />
        <NuxtPage />
      </NNotificationProvider>
    </template>
  4. Use the dispatch utility in places you cannot use the regular useNotification() of naive-ui:

    import dispatchNotification from '../components/TheEventBasedNotification/dispatch'
    
    export default defineNuxtPlugin(() => {
      // this works now 🎊 
      dispatchNotification({
        type: 'error',
        title: "notification",
        content: "hello",
      })
    }})