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

Fix return types for backend resource deletions #3833

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

Conversation

kylekz
Copy link

@kylekz kylekz commented Jul 29, 2024

Description

When deleting resources, for example an Organization, the backend client is typed to return Organization, whereas the actual HTTP API returns a DeletedObject, resulting in a type mismatch. This PR:

  • updates DeletedObjectJSON to accept a generic that extends string, with a default of string
    • i initially wanted this to extend ObjectType but then that would break DeletedObject.fromJSON()
  • changes the generics passed into this.request() for the following resource delete methods:
    • Allowlist
    • Domain
    • EmailAddress
    • Organization
    • PhoneNumber
    • RedirectUrl
    • SamlConnection
      • for this one i'm just assuming ObjectType.SamlAccount is the correct type, as there's no SamlConnection
    • User

I'm not sure what the convention would be for handling this DeletedObjectJSON type property so this just seemed like the most straight forward way that wouldn't break anything 😅

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented Jul 29, 2024

🦋 Changeset detected

Latest commit: 8716f30

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@clerk/backend Patch
@clerk/astro Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/tanstack-start Patch
@clerk/testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@dimkl dimkl left a comment

Choose a reason for hiding this comment

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

Hello @kylekz and sorry for the late response and thank you very much for the contribution but i am afraid that it will require some work in order to be merged.

Since you took the time to contribute i will try to provide some context for the rest of the required changes.
The API methods of our backend SDK are expected to return a *Resource type. The request<T> method used is expected to be passed a *Resource type which will be the returned type of the API method.
Based on the above instead of using the this.request<DeletedObjectJSON<typeof ObjectType.*>> we should be using the this.request<DeletedObject<typeof ObjectType.SamlAccount>>

Finally the saml_account and saml_connection are different resources. As you have noticed the ObjectType.SamlConnection is missing from our codebase so we need to add it 😅

@kylekz
Copy link
Author

kylekz commented Aug 8, 2024

@dimkl no problem, thanks for the response! I've updated to add ObjectType.SamlConnection, added a generic to DeletedObject and made sure to use that instead of DeletedObjectJSON. The generics extend ObjectType instead of string now so they're required, which doesn't break any existing types but I'd imagine could possibly introduce a breaking change to anyone depending on DeletedObject and/or DeletedObjectJSON in application code.

I don't want to be wasting dev time with reviews etc so I'm off the mark here then I'm cool with closing and just opening an issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants