Skip to content

feat(backend): Also export token-related errors#1980

Closed
flawnn wants to merge 2 commits into
clerk:mainfrom
flawnn:feat/tokenexport
Closed

feat(backend): Also export token-related errors#1980
flawnn wants to merge 2 commits into
clerk:mainfrom
flawnn:feat/tokenexport

Conversation

@flawnn

@flawnn flawnn commented Oct 30, 2023

Copy link
Copy Markdown
Contributor

Description

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.

Type of change

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

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

@changeset-bot

changeset-bot Bot commented Oct 30, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1cef899

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

This PR includes changesets to release 6 packages
Name Type
@clerk/backend Minor
@clerk/fastify Patch
gatsby-plugin-clerk Patch
@clerk/nextjs Patch
@clerk/remix Patch
@clerk/clerk-sdk-node 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

@tmilewski tmilewski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@flawnn Thanks for your PR! Looks good to me! Would you mind adding a changeset?

@flawnn

flawnn commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

@tmilewski Done!

@nikosdouvlis nikosdouvlis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello @flawnn :)
This PR makes total sense to me, however, it also exposes a few internal errors that should probably remain internal for the time being.

Could you please share more details about your use case so we can better understand the changes we need to make?

@LekoArts

Copy link
Copy Markdown
Contributor

Please also add a description to your PR. Thanks!

@flawnn

flawnn commented Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Hello @flawnn :) This PR makes total sense to me, however, it also exposes a few internal errors that should probably remain internal for the time being.

Could you please share more details about your use case so we can better understand the changes we need to make?

Hey! The reason why I was suggesting this PR was that I wanted to be able to catch JWT Verification errors when working with verifyJwt. As an example here a code snippet:

try {
    let v = await verifyJwt(authHeader.split(" ")[1], {} as VerifyJwtOptions);

    next();
  } catch (e) {
    if (e instanceof TokenVerificationError) {
	  // Logic here doesn't make sense as there are multiple reasons obviously but just for sake of testing
      return res.status(401).send("Token has expired");
    }
    return res.status(401).send("Invalid token");
  }

Maybe as a reason why I even decided doing that, is that the Clerk Middleware for Express actually just throws an error and doesn't pass that error on to following middlewares when decoding the JWT Token (in some cases). One case was, when the JWT Token expired. The problem with not passing it on to following middlewares is obviously that it doesn't allow for custom error handling.

EDIT:

https://clerk.com/docs/backend-requests/handling/nodejs#express-error-handlers

The paragraph here basically isn't true. I am not able to handle all errors apparently.

@tmilewski
tmilewski changed the base branch from main to release/v4 November 17, 2023 21:10
@tmilewski
tmilewski requested a review from a team as a code owner November 17, 2023 21:10
@tmilewski
tmilewski changed the base branch from release/v4 to main November 17, 2023 21:13
@tmilewski
tmilewski removed the request for review from a team November 17, 2023 21:15
@tmilewski
tmilewski changed the base branch from main to release/v4 November 17, 2023 21:40
@tmilewski
tmilewski changed the base branch from release/v4 to main November 17, 2023 21:41
@nikosdouvlis

Copy link
Copy Markdown
Member

@flawnn, many thanks for the contribution and inciting an internal discussion about this!
I'm going to close this PR as it targets main which is the development branch of our next major release (v5)
@tmilewski will be backporting the change into v4 (the current version of our packages) - you can track the progress here: #2189

Thank you :)

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.

5 participants