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

chore(backend,nextjs,clerk-sdk-node): Drop legacy return response in BAPI responses #2126

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Nov 13, 2023

Description

Currently the BAPI responses are in { data: unknown | null, errors: [] | null } format but to avoid introducing a breaking change we convert those responses and return only the data part or throw an error if there are errors.
With this PR we will drop the transformation and return the actual response. This is a breaking change since from now on

  • no error will be thrown from the BAPI requests client
  • the caller will have to check for the errors prop in the return value to verify if the request succeeded
  • the caller will have to retrieve the response data from the data prop in the return value

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:

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

@dimkl dimkl self-assigned this Nov 13, 2023
@dimkl dimkl requested a review from a team as a code owner November 13, 2023 21:24
Copy link

changeset-bot bot commented Nov 13, 2023

🦋 Changeset detected

Latest commit: fa0aa4b

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

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

if (!payload) {
assert.false(true, 'This assertion should never fail. We need to check for payload to make TS happy.');
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assertResponse / assertErrorResponse helpers were introduced to drop the !payload check.

@@ -38,7 +38,12 @@ export const createClerkExpressRequireAuth = (createOpts: CreateClerkExpressMidd
clerkClient,
requestState,
});
return handleInterstitialCase(res, requestState, interstitial);
if (interstitial.errors) {
// TODO(@dimkl): return interstitial errors ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikosdouvlis WDYT about passing the errors of the interstitial request to the end-user?
We would probably remove this handling if we drop the remotePrivateInterstitial().

Copy link
Member

Choose a reason for hiding this comment

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

Did we remove remotePrivateIntestitial?
Also, what errors can be returned here? Im a bit skeptical to pass the errors to the user as that makes the error a public API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't removed it yet, since we need to decide if we want to enforce the publishableKey in all our SDKs.
The local interstitial won't ever return an error but if the publishableKey is missing, fetching interstitial from BAPI will be triggered and that could result to the following errors (based on the Go backend):

  • 401 due to missing secretKey or invalid Key
  • 500

Copy link
Member

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Rest LGTM

.changeset/mighty-pugs-knock.md Show resolved Hide resolved
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch from 9be55f9 to 07edf5b Compare November 14, 2023 15:37
@dimkl dimkl force-pushed the sdk-91-drop-legacy-response branch from bbdb0cd to 8647965 Compare November 14, 2023 16:20
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch 3 times, most recently from 6b8d70b to 92bbeb9 Compare November 15, 2023 14:16
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch from 92bbeb9 to 53bef9c Compare November 16, 2023 10:47
Base automatically changed from sdk-782-drop-deprecations-backend to main November 16, 2023 11:41
@dimkl dimkl force-pushed the sdk-91-drop-legacy-response branch 2 times, most recently from 28244ea to 19cb42b Compare November 16, 2023 12:11
@dimkl dimkl requested a review from LekoArts November 16, 2023 12:11
@dimkl dimkl force-pushed the sdk-91-drop-legacy-response branch from 9dc8bfc to b5196ad Compare November 16, 2023 17:08
@dimkl dimkl force-pushed the sdk-91-drop-legacy-response branch from b5196ad to e50a198 Compare November 17, 2023 00:54
@@ -38,7 +38,12 @@ export const createClerkExpressRequireAuth = (createOpts: CreateClerkExpressMidd
clerkClient,
requestState,
});
return handleInterstitialCase(res, requestState, interstitial);
if (interstitial.errors) {
// TODO(@dimkl): return interstitial errors ?
Copy link
Member

Choose a reason for hiding this comment

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

Did we remove remotePrivateIntestitial?
Also, what errors can be returned here? Im a bit skeptical to pass the errors to the user as that makes the error a public API

@dimkl dimkl force-pushed the sdk-91-drop-legacy-response branch from e50a198 to fa0aa4b Compare November 17, 2023 08:43
@dimkl dimkl added this pull request to the merge queue Nov 17, 2023
Merged via the queue into main with commit dd57030 Nov 17, 2023
6 checks passed
@dimkl dimkl deleted the sdk-91-drop-legacy-response branch November 17, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants