remix-pwa / monorepo

Remix PWA v4
https://remix-pwa.run
MIT License
128 stars 25 forks source link

Push errors #271

Open Superalexandre opened 1 month ago

Superalexandre commented 1 month ago

When I try to generate a generateSubscriptionId I got an error :

TypeError: genSalt is not a function
    at Module.generateSubscriptionId (PATH/node_modules/@remix-pwa/push/dist/server/utils.js:11:24)
    at action (PATH\app\routes\api\notifications\subscribe\index.ts:22:34)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    at async Object.callRouteAction (PATH\node_modules\@remix-run\server-runtime\dist\data.js:36:16)
    at async PATH\node_modules\@remix-run\router\dist\router.cjs.js:4724:19
    at async callLoaderOrAction (PATH\node_modules\@remix-run\router\dist\router.cjs.js:4790:16)
    at async Promise.all (index 0)
    at async defaultDataStrategy (PATH\node_modules\@remix-run\router\dist\router.cjs.js:4649:17)
    at async callDataStrategyImpl (PATH\node_modules\@remix-run\router\dist\router.cjs.js:4681:17)
    at async callDataStrategy (PATH\node_modules\@remix-run\router\dist\router.cjs.js:4136:19)
    at async submit (PATH\node_modules\@remix-run\router\dist\router.cjs.js:3995:21)
    at async queryImpl (PATH\node_modules\@remix-run\router\dist\router.cjs.js:3953:22)
    at async Object.queryRoute (PATH\node_modules\@remix-run\router\dist\router.cjs.js:3922:18)
    at async handleResourceRequest (PATH\node_modules\@remix-run\server-runtime\dist\server.js:402:20)
    at async requestHandler (PATH\node_modules\@remix-run\server-runtime\dist\server.js:156:18)
    at async nodeHandler (PATH\node_modules\@remix-run\dev\dist\vite\plugin.js:839:27)
    at async PATH\node_modules\@remix-run\dev\dist\vite\plugin.js:842:15

Second thing, when I use the function sendNotifications and the subscriptions is invalid (for exemple the user have subscribed then unistall the pwa and the subscriptions is still in database) The whole process crash even in a try catch it could be nice to throw the error without crash

ShafSpecs commented 1 month ago

What's your send notification block like? And are you using CJS or ESM?

Superalexandre commented 1 month ago
    for (const subscription of allSubscriptions) {
        console.log("Sending notification to", subscription.endpoint)

        try {
            sendNotifications({
                notification: {
                    title: "Title",
                    options: {
                        body: "Body",
                        data: {
                            url: "/"
                        }
                    }
                },
                vapidDetails: {
                    publicKey: process.env.NOTIFICATION_PUBLIC_KEY as string,
                    privateKey: process.env.NOTIFICATION_PRIVATE_KEY as string,
                },
                subscriptions: [
                    {
                        endpoint: subscription.endpoint,
                        keys: {
                            p256dh: subscription.p256dh,
                            auth: subscription.auth,
                        }
                    }
                ],
                options: {}
            })
        } catch (error) {
            console.error("Error while sending notification", error) // Dont go here
        }
    }

And im using ESM

Btw another question there is a way to optimize this ?

  useEffect(() => {
        if ("serviceWorker" in navigator) {
            navigator.serviceWorker.addEventListener("message", (event) => {
                console.log("SW message", event)

                if (event.data && event.data.type === "notification") {
                    toast({
                        title: event.data.title,
                        description: event.data.body,
                        action: (
                            <ToastAction
                                altText="Ouvrir"
                                onClick={() => navigate(event.data.url)}
                            >
                                Ouvrir
                            </ToastAction>
                        )
                    })
                }
            })
        }
    }, [])
ShafSpecs commented 1 month ago
    for (const subscription of allSubscriptions) {
        console.log("Sending notification to", subscription.endpoint)

        try {
            sendNotifications({
                // ...
            })
        } catch (error) {
            console.error("Error while sending notification", error) // Dont go here
        }
    }

This is pretty weird, because I throw the error on purpose so it could get caught: https://github.com/remix-pwa/monorepo/blob/6fa72a544991a59f8b384a733746a6ec65af75f5/packages/push/server/notifications.ts#L16-L25

And im using ESM

Btw another question there is a way to optimize this ?

useEffect(() => {
      if ("serviceWorker" in navigator) {
          navigator.serviceWorker.addEventListener("message", (event) => {
              console.log("SW message", event)

              if (event.data && event.data.type === "notification") {
                  toast({
                      title: event.data.title,
                      description: event.data.body,
                      action: (
                          <ToastAction
                              altText="Ouvrir"
                              onClick={() => navigate(event.data.url)}
                          >
                              Ouvrir
                          </ToastAction>
                      )
                  })
              }
          })
      }
  }, [])

No, there isn't. When you say optimise, do you mean whether Remix PWA offers something out of the box to shorten it?

Superalexandre commented 1 month ago
 subscriptions.forEach(subscription => { 
   webpush 
     .sendNotification(subscription, JSON.stringify(notification), { ...options, vapidDetails: details }) 
     .then((result: { statusCode: any }) => { 
       return result; 
     }) 
     .catch((error: any) => { 
       throw new Error(error); 
     }); 
 }); 

I cant even get the result data, typescript says its an void function

And in dev mode, so with vite it make it crash But in prod with an hono server just an error still cant catch but no crash

No, there isn't. When you say optimise, do you mean whether Remix PWA offers something out of the box to shorten it?

Tbh i would be prettier and easier if Remix PWA could offer something

ShafSpecs commented 1 month ago

Oh, I think I see why. I would add this to the v5 pile

I just realised why it is broken 😅

ShafSpecs commented 1 month ago

Tbh i would be prettier and easier if Remix PWA could offer something

👍

Superalexandre commented 1 month ago

Oh, I think I see why. I would add this to the v5 pile

I just realised why it is broken 😅

Ahah okay perfect then, do you have an estimation when it will be ready ?

And while im here, I dont know if its planned but there is missing types in the WebAppManifest for exemple

In the display_override we can only put 'window-controls-overlay' | 'bordered' | 'standard' but there is more options (https://developer.mozilla.org/en-US/docs/Web/Manifest/display_override) like fullscreen, tabbed

And the screenshots params is missing,

screenshots: Array<{
        src: string
        type?: string
        sizes?: string
        form_factor?: "wide" | "full-screen" | "any"
        label?: string
}>

(https://developer.mozilla.org/en-US/docs/Web/Manifest/screenshots)

The id (string) params too (https://developer.mozilla.org/en-US/docs/Web/Manifest/id)

I could provide a pull request for this one if you like

And the ManifestLink component doesnt provide the <link rel="manifest" href="/manifest.webmanifest" /> but provide the <link rel="webmanifest" href="/manifest.webmanifest" /> correctly (maybe ive done something wrong on this one)

ShafSpecs commented 1 month ago

Regarding the fix, tbh I am not certain. I am currently re-working a lot of Remix PWA features, and I have to write a whole new docs for it. But I would try and get a release candidate you can download out ASAP. Remix PWA now has a manifest package btw @remix-pwa/manifest - if you are going to open a PR, I would suggest opening it on the dev branch and within the manifest package

Superalexandre commented 1 month ago

Okay perfect thank you so much, and thanks for the work i love the package !

ShafSpecs commented 1 month ago

Sorry @Superalexandre, was super busy, but moving the changes to dev, so you can install the changes via npm i @remix-pwa/push@dev

ShafSpecs commented 1 month ago

https://github.com/remix-pwa/monorepo/actions/runs/11498149659/job/32003337173 - pushed to the dev version release

Superalexandre commented 1 month ago

Hey, no worries, i will check this soon, thanks for the work !