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

refactor(IntegrationApplication): move common properties to Application #10627

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

imnaiyar
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:
Moved common properties that are present for both ClientApplication and IntegrationApplication to Application as they both extend it.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@imnaiyar imnaiyar requested a review from a team as a code owner November 28, 2024 13:52
Copy link

vercel bot commented Nov 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 9:41pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Dec 5, 2024 9:41pm

@imnaiyar imnaiyar force-pushed the refactor-integration-application branch from b40525e to 839fa78 Compare November 28, 2024 13:54
Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

From looking at the specification, the only common properties between all application responses are id, name, and description. In our Application class, name and description are nullable, but they are always present in every application schema response (ApplicationResponse, BasicApplicationResponse, IntegrationApplicationResponse, InviteApplicationResponse, and PrivateApplicationResponse). summary is an empty string if none is set.

I guess what I'm trying to say is this pull request has brought to light that we should probably have different classes for these different response types. For example, getting the target application of an invite should return an InviteApplication—not an IntegrationApplication. IntegrationApplication should be reserved for actual integration applications that come from fetching audit logs or explicitly fetching them. If we do this, we can narrow the types down because what's always present differs:

InviteApplicationResponse: https://github.com/discord/discord-api-spec/blob/db5fe6cf36441a78d93fe059cfb2f0e3ba266232/specs/openapi.json#L21615-L21621

IntegrationApplicationResponse: https://github.com/discord/discord-api-spec/blob/db5fe6cf36441a78d93fe059cfb2f0e3ba266232/specs/openapi.json#L21158-L21162

Here, you'll see integration applications do not have terms_of_service_url or privacy_policy_url, so moving them to the base class to which all inherit is technically inaccurate, albeit fine right now because there is no distinction between these two responses.

This pull request is fine as-is, I suppose, but this is something to think about.


Also, hook is not documented anywhere I can see. It is not in the specification or the documentation. Sounds like it should be removed.

@imnaiyar
Copy link
Contributor Author

Hmm, I'll have to re-write this PR if that's what we want to do then.

In our Application class, name and description are nullable, but they are always present in every application schema response

As per Qjuh response here #10588 (comment), the ready event sends partial application object that only includes the id, and flags. So we'll have to decide how to handle that since it can be missing

@Jiralite
Copy link
Member

This is fine as-is. My thoughts sound like a lot of effort, so it's probably something to think about in the future.

@imnaiyar
Copy link
Contributor Author

Future someone else's problem then 😅. I should remove hook then, but that would probably make the pr breaking ig

@Jiralite
Copy link
Member

To keep the scope, yeah, best to do it in another pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

2 participants