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

Node SDK: verifyToken function has wrong signature #2280

Closed
4 tasks done
mboudreau opened this issue Dec 7, 2023 · 1 comment
Closed
4 tasks done

Node SDK: verifyToken function has wrong signature #2280

mboudreau opened this issue Dec 7, 2023 · 1 comment
Assignees
Labels
prioritized This issue has been triaged and the team is working on it

Comments

@mboudreau
Copy link

Preliminary Checks

Reproduction / Replay Link

https://github.com/clerk/javascript/blob/main/packages/sdk-node/src/clerkClient.ts#L11

Publishable key

yeah, nah

Description

Steps to reproduce:

  1. install @clerk/clerk-sdk-node@4.12.23
  2. Add code that calls clerkClient.verifyToken(token)
  3. Watch as typescript throws a fit because it's expecting 2 arguments. Weird.
  4. Add options object as second parameter. Watch typescript throw a tantrum asking for the issuer to be specified even though it shouldn't be required.

Expected behavior:

The signature should be able to accept a single argument as per your code, but because the (ExtendedClerk type references @clerk/backend for the verifyToken type)[https://github.com/clerk/javascript/blob/main/packages/sdk-node/src/clerkClient.ts#L11] and since that signature is a broken, it breaks the child.

I personally find it weird that the backend code requires the second argument with empty object fallback as all the properties inside this options object should be optional except for some reason, the issuer hasn't been made optional in the option signature, even though it is optional in the code for verifyToken.

Furthermore, code like this type signature makes it extremely hard to read and gives very little value to your developers other than not having to copy some types over, but the issue now lies that the base type has immense power over your SDK signature which can break things as clearly shown here. Having 2 picks and your own extension to the type shows that your types are not well thought out and should be reassessed for better abstraction, or maybe it might be easier to just have an explicit type for each function to prevent these kinds spaghetti type issues in the future.

Actual behavior:

verifyToken signature won't allow single token argument without breaking.

Environment

Doesn't matter
@mboudreau mboudreau added the needs-triage A ticket that needs to be triaged by a team member label Dec 7, 2023
@dimkl dimkl self-assigned this Dec 7, 2023
dimkl added a commit that referenced this issue Dec 7, 2023
@dimkl dimkl added prioritized This issue has been triaged and the team is working on it and removed needs-triage A ticket that needs to be triaged by a team member labels Dec 7, 2023
dimkl added a commit that referenced this issue Dec 7, 2023
github-merge-queue bot pushed a commit that referenced this issue Dec 7, 2023
* fix(backend): Add `createdAt` param in User#createUser request

Resolves #2281

* fix(clerk-sdk-node): Fix `verifyToken()` signature to support a signle token param

Resolves #2280

* chore(repo): Add changeset
dimkl added a commit that referenced this issue Dec 7, 2023
* fix(backend): Add `createdAt` param in User#createUser request

Resolves #2281

* fix(clerk-sdk-node): Fix `verifyToken()` signature to support a signle token param

Resolves #2280

* chore(repo): Add changeset

(cherry picked from commit 7af0949)
github-merge-queue bot pushed a commit that referenced this issue Dec 7, 2023
#2288)

* fix(backend): Add `createdAt` param in User#createUser request

Resolves #2281

* fix(clerk-sdk-node): Fix `verifyToken()` signature to support a signle token param

Resolves #2280

* chore(repo): Add changeset

(cherry picked from commit 7af0949)
@dimkl
Copy link
Contributor

dimkl commented Dec 7, 2023

Hello @mboudreau
thank you for reporting this. A fix has been released as part of @clerk/clerk-sdk-node@4.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prioritized This issue has been triaged and the team is working on it
Projects
None yet
Development

No branches or pull requests

2 participants