Skip to content
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

Push errors #271

Open
Superalexandre opened this issue Oct 7, 2024 · 12 comments · Fixed by #274
Open

Push errors #271

Superalexandre opened this issue Oct 7, 2024 · 12 comments · Fixed by #274
Assignees
Labels
bug Something isn't working

Comments

@Superalexandre
Copy link

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 ShafSpecs self-assigned this Oct 11, 2024
@ShafSpecs ShafSpecs added the bug Something isn't working label Oct 11, 2024
@ShafSpecs
Copy link
Member

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

@Superalexandre
Copy link
Author

Superalexandre commented Oct 11, 2024

    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
Copy link
Member

    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:

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);
});
});

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
Copy link
Author

Superalexandre commented Oct 11, 2024

 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
Copy link
Member

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

I just realised why it is broken 😅

@ShafSpecs
Copy link
Member

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

👍

@Superalexandre
Copy link
Author

Superalexandre commented Oct 11, 2024

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
Copy link
Member

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
Copy link
Author

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

@ShafSpecs ShafSpecs linked a pull request Oct 24, 2024 that will close this issue
@ShafSpecs
Copy link
Member

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
Copy link
Member

@Superalexandre
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants