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

feat: openExternalLink returns a { opened: boolean } result #282

Merged

Conversation

HamzaAtDiscord
Copy link
Collaborator

Edit the return type for openExternalLink. With this change the result of openExternalLink will be an object containing a boolean value opened.

If opened is true, then the link was opened by the user
If opened is false, then the link was not opened by the user

The lifetime of the promise is also changed. Previously the promise completed after opening the "Leaving Discord" dialog, but with this change the promise completes after the user selects an action in the dialog (visit the site, or go back). If the dialog does not appear because the site visit is automatically approved, then the promise returns { opened: true } following the link opening.

@HamzaAtDiscord HamzaAtDiscord force-pushed the hamzahutchinson/change-open-external-link-response branch from b030381 to 2faa932 Compare November 15, 2024 01:36
Comment on lines 109 to 112
export const OpenExternalLinkResponse = zod.object({
opened: zod.boolean(),
});

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general question: how does this behave against a version of the client that doesn't send the opened flag across RPC? my gut is that since this isn't marked as optional and we're using .parse() it would error out, but I also don't know if we have error handling for that buried elsewhere (I'm assuming we do since otherwise I imagine this would have been a problem already).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good callout

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would defaulting this to false be appropriate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've updated this to use fallbackToDefault and return { opened: null } in the case where parsing fails (ie: discord clients that don't support this response). I've also updated the documentation with the null case and a note on when it occurs.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@discord/embedded-app-sdk",
"version": "1.7.0",
"version": "1.7.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - we don't bump the package version in the PR, it happens automatically via the release please github action.

nit - I think this is a feature, not a patch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't realize! I have reverted to 1.7.0

Copy link
Collaborator

@afgiel afgiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as jae called out we need to consider how this will work with older clients, especially on mobile. we may need to add optional to the OpenExternalLinkResponse or wait until the version has saturated adoption

Comment on lines 109 to 112
export const OpenExternalLinkResponse = zod.object({
opened: zod.boolean(),
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good callout

@HamzaAtDiscord HamzaAtDiscord force-pushed the hamzahutchinson/change-open-external-link-response branch from 24b227d to 13e5550 Compare November 18, 2024 21:33
@matthova matthova changed the title openExternalLink returns a { opened: boolean } result feat: openExternalLink returns a { opened: boolean } result Nov 19, 2024
Copy link
Contributor

@matthova matthova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the linting error is resolved, lgtm

@HamzaAtDiscord HamzaAtDiscord merged commit d3bd375 into main Nov 19, 2024
4 checks passed
@HamzaAtDiscord HamzaAtDiscord deleted the hamzahutchinson/change-open-external-link-response branch November 19, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants