search

discord / embedded-app-sdk

🚀 The Discord Embedded App SDK lets you build rich, multiplayer experiences as Activities inside Discord.
https://discord.com/developers/docs/activities/overview
MIT License
1.24k stars 175 forks source link

discordSdk.ready() never completes after full page reload. #41

Closed alula closed 5 months ago

alula commented 6 months ago

While prototyping an app, I've found out that discordSdk.ready() never completes after full page reload. The issue has also been mentioned in #activities-dev-help channel on Discord Developers server.

How to reproduce:

  1. This issue is even reproducible in the starter application: https://github.com/discord/embedded-app-sdk/tree/main/examples/discord-activity-starter
  2. pnpm dev
  3. Join the activity
  4. Save main.ts to make Vite reload, do window.location.reload() or whatever else from your application.

I've tested and reproduced the issue on following Discord clients:

DaniDipp commented 6 months ago

I ran into the same issue trying to reload for debugging. My minimum reproduction as a single html file:

<html>
<head>
<script type="module">
  const { DiscordSDK } = await import("https://unpkg.com/@discord/embedded-app-sdk@1.0.0/output/index.mjs");
  const discordSdk = new DiscordSDK(CLIENT_ID);

  document.body.innerHTML += "<p>Waiting for DiscordSDK to be ready...</p>";
  await discordSdk.ready();

  document.body.innerHTML += "<p>DiscordSDK is ready! Reloading in 2 seconds.</p>";
  await new Promise(res => setTimeout(res, 2000));

  window.location.reload();
</script>
</head>
<body></body>
</html>
alula commented 6 months ago

I think I've found some clues!

The SDK uses document.referrer to figure out the origin of parent window to limit it to the only recipient of RPC messages (in theory):

https://github.com/discord/embedded-app-sdk/blob/5cc3da806a8bc35b3a3b3c289975c7f14e662591/src/Discord.ts#L39-L41

Before reload the value of document.referrer is https://discord.com/.

After reload the value of document.referrer is equal to URL of your iframe, which is passed to postMessage(), which means the application sends messages to itself instead of Discord client.

Technically you can mess with sourceOrigin property of Discord SDK (note: this impacts security, I discourage doing this on production!!):

const discordSdk = new DiscordSDK(import.meta.env.VITE_CLIENT_ID, { disableConsoleLogOverride: true });
if (import.meta.env.DEV) {
    const discordSdkAny = discordSdk as any;
    if (!discordSdkAny.sourceOrigin.includes("discord.com")) {
        discordSdkAny.sourceOrigin = "*";
        discordSdkAny.handshake();
    }
}

However, Discord client doesn't seem to like getting a handshake from reloaded frame:

image

alula commented 6 months ago

Even hacking it to set isReady to true, makes commands like discordSdk.commands.authorize() fail with "Already authenticated" error.

Working around this seems to require the formerly mentioned hack and caching of authentication/user data, eg. in SessionStorage.

The problem seems to lie in both the library and clients.

This is hacky and horrible but might be useful for someone: https://gist.github.com/alula/2e9f6874bd7f12d4ca9201347979ac92

real2two commented 6 months ago

this really needs to be fixed, and it really negatively impacts developer experience.

my current "workaround" is to use DiscordSDKMock and test the activity through a browser instead of Discord, instead of trying to do some hacky solution. (but this doesn't fix the hot reloading issue.)

DiscordSDKMock reference: https://github.com/discord/embedded-app-sdk/blob/main/examples/react-colyseus/packages/client/src/discordSdk.tsx

AshMW2724 commented 6 months ago

I believe they really only intended this to be used with single pages apps which do not do full page refreshes. With my SPA I use hot reloading in development and it sometimes required a full page refresh so I listen to onbeforeunload to run discordSdk.close().

DaniDipp commented 6 months ago

I listen to onbeforeunload to run discordSdk.close().

That closes the activity for me. You got it to work with a location.reload()?

AshMW2724 commented 6 months ago

No, what I am saying is just force the activity to close when a reload is detected. https://github.com/AshMW2724/react-discord-activity/blob/main/src/hooks/useAuthenticatedContext.tsx#L49

alula commented 6 months ago

The client allows only one handshake to be performed and when the window is reloaded the fake RPC connection is kept in same state, client is unaware of anything that happened, but your app is losing all the state.

The only workaround for now to support reloads/redirects is to somehow keep the state of discord authentication (eg. through sessionStorage) and use the workaround for the origin bug in app SDK.

real2two commented 6 months ago

Even hacking it to set isReady to true, makes commands like discordSdk.commands.authorize() fail with "Already authenticated" error.

Working around this seems to require the formerly mentioned hack and caching of authentication/user data, eg. in SessionStorage.

The problem seems to lie in both the library and clients.

This is hacky and horrible but might be useful for someone: https://gist.github.com/alula/2e9f6874bd7f12d4ca9201347979ac92

does this work on mobile as well? I'm using LocalStorage as well now, but I wrote my own code for it. I could be wrong, but I think every time I initialize DiscordSDK, call the ready function, then initalize the class again, the activity automatically closes on mobile (android).

matthova commented 5 months ago

Unfortunately, the SDK (and the matching RPC server which it is talking to) is designed to support only one "ready handshake" per iframe mounted. Once the handshake between a Discord client and the iframe is complete, it will not issue additional "READY" payloads. As noted here activities are intended for "single page apps" (SPAs).

This means your activity needs to only initialize the SDK once per activity instance.

When it comes to a smooth developer flow, one optimization to explore is isolating code which calls new DiscordSDK() and discordSdk.ready from the rest of your flow to reduce the chance of waiting for reinitialization of the SDK.

Pkmmte commented 5 months ago

Unfortunately, the SDK (and the matching RPC server which it is talking to) is designed to support only one "ready handshake" per iframe mounted. Once the handshake between a Discord client and the iframe is complete, it will not issue additional "READY" payloads.

I appreciate this detail and was able to reference the Colyseus example alongside it to find a clean solution! ✨

For those interested, you can avoid having Vite's HMR re-create the SDK instance in a React app by managing it in a context provider close to the root node. This is what the react-colyseus and sdk-playground examples do to avoid this issue.

My team has a minimal example of this in our framework's template example for more... context: https://github.com/Wave-Play/robo.js/tree/main/templates/starter-app-ts-react#authenticating

MDFL64 commented 5 months ago

I do really prefer to have the option to do a full reload in some scenarios, like in the event of an update or an unrecoverable error.

Any chance the promise could resolve to an error instead of just hanging?

Jimbly commented 5 months ago

I'm also struggling greatly without a functioning reload. I'm currently tasked with getting Worlds FRVR into Discord activities, and we reload under a number of circumstances. In development, the obvious case of applying any changes that can't be hot-reloaded, so not having it there is rather annoying (but not a show stopper, even if physically painful, as I have to reach for the mouse to restart the activity). In production, we do reloads when we apply updates (absolutely critical, as, if it's a "breaking"/protocol change, old clients will simply not work, and even if a non-breaking change, old clients are going to be loading new data which will lead to undefined behavior). We also do reloads in production to deal with other rare situations that can't be reasonably gracefully handled (e.g. user changing antialiasing settings, which requires re-creating the canvas/WebGL context and all dynamic GPU data). Without reloads functioning we're just going to have do the discordSDK.close() approach, which is a pretty crappy experience for the end-user (and frustrating for development).

MDFL64 commented 5 months ago

The recommendation seems to be to make another nested iframe, which seems yucky to me. I was able to work around it in my simple prototype, but it seems my only viable option for updates is to either the nested iframes or killing the activity as you suggest.