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): Drop deprecations #1899

Merged
merged 15 commits into from
Nov 16, 2023
Merged

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Oct 17, 2023

Description

Drop deprecated properties. Migration steps:

  • use createClerkClient instead of __unstable_options
  • use publishableKey instead of frontendApi
  • use clockSkewInMs instead of clockSkewInSeconds
  • use apiKey instead of secretKey
  • drop httpOptions
  • use *.image instead of
    • ExternalAccount.picture
    • ExternalAccountJSON.avatar_url
    • Organization.logoUrl
    • OrganizationJSON.logo_url
    • User.profileImageUrl
    • UserJSON.profile_image_url
    • OrganizationMembershipPublicUserData.profileImageUrl
    • OrganizationMembershipPublicUserDataJSON.profile_image_url
  • drop pkgVersion
  • use Organization.getOrganizationInvitationList with status instead of getPendingOrganizationInvitationList
  • drop orgs claim (if required, can be manually added by using user.organizations in a jwt template)

Internal changes:

  • replaced error enum (and it's) SetClerkSecretKeyOrAPIKey with SetClerkSecretKey

TODO

  • drop Interstitial deprecation (will be done in a separate PR since we will probably keep only the local interstitial based on the SDK public APIs for v5 RFC)

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/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

@dimkl dimkl self-assigned this Oct 17, 2023
@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2023

🦋 Changeset detected

Latest commit: 53bef9c

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 Major
@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

@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch from 505650d to 57684e7 Compare October 19, 2023 13:32
@dimkl dimkl marked this pull request as ready for review October 19, 2023 13:33
@dimkl dimkl requested a review from a team as a code owner October 19, 2023 13:33
@dimkl dimkl marked this pull request as draft October 19, 2023 13:33
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch 3 times, most recently from 4d64e56 to f95ab26 Compare October 26, 2023 21:24
@dimkl dimkl changed the title chore(backend): Drop clerk@v4 deprecations chore(backend): Drop deprecations Oct 31, 2023
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch from f95ab26 to 526a49c Compare November 7, 2023 12:52
@dimkl dimkl marked this pull request as ready for review November 7, 2023 12:57
@dimkl dimkl closed this Nov 7, 2023
@dimkl dimkl reopened this Nov 7, 2023
@dimkl dimkl closed this Nov 7, 2023
@dimkl dimkl deleted the sdk-782-drop-deprecations-backend branch November 7, 2023 13:27
@dimkl dimkl restored the sdk-782-drop-deprecations-backend branch November 7, 2023 13:27
@dimkl dimkl deleted the sdk-782-drop-deprecations-backend branch November 7, 2023 13:27
@dimkl dimkl restored the sdk-782-drop-deprecations-backend branch November 7, 2023 13:28
@dimkl dimkl reopened this Nov 7, 2023
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch 3 times, most recently from c9f2381 to 18dabbc Compare November 8, 2023 13:36
const noSchemeFrontendApi = frontendApi.replace(/http(s)?:\/\//, '');
const major = getClerkJsMajorVersionOrTag(frontendApi, pkgVersion);
const major = getClerkJsMajorVersionOrTag(frontendApi, clerkJSVersion);
return `https://${noSchemeFrontendApi}/npm/@clerk/clerk-js@${clerkJSVersion || major}/dist/clerk.browser.js`;
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 Since we are dropping the pkgVersion we won't be able to load the major version of a version provided implicitly because the clerkJSVersion || major where the major version is being calculated using the clerkJSVersion.
This is the reason behind dropping the test in packages/shared/src/__tests__/url.test.ts.

We will be able to use:

  • the latest or canary tag or
  • the explicit version passed

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I undertand why we are making this change. Could you please share more context? We need to be able to load version 4 for example, otherwise each clerk-react major version will not be able to load the correct clerk-js package.

We cannot rely on latest for this - what will happen when we release v6?

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 the clerk-react does not use this script to load the clerk-js. This is used to load ClerkJS in interstitial and it seems to be an existing issue that needs to be solved.
I will merge this an open a ticket to validate and handle the clerkJS version loaded in the interstitial

@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch 6 times, most recently from a6d5666 to 6b8d70b Compare November 15, 2023 12:53
Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

A few minor comments and a question - appart from that, everything looks good. Approving in order to unblock!

packages/backend/src/tokens/factory.ts Show resolved Hide resolved
packages/backend/src/tokens/interstitialRule.ts Outdated Show resolved Hide resolved
packages/backend/src/tokens/interstitialRule.ts Outdated Show resolved Hide resolved
const noSchemeFrontendApi = frontendApi.replace(/http(s)?:\/\//, '');
const major = getClerkJsMajorVersionOrTag(frontendApi, pkgVersion);
const major = getClerkJsMajorVersionOrTag(frontendApi, clerkJSVersion);
return `https://${noSchemeFrontendApi}/npm/@clerk/clerk-js@${clerkJSVersion || major}/dist/clerk.browser.js`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I undertand why we are making this change. Could you please share more context? We need to be able to load version 4 for example, otherwise each clerk-react major version will not be able to load the correct clerk-js package.

We cannot rely on latest for this - what will happen when we release v6?

@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch from 6b8d70b to 92bbeb9 Compare November 15, 2023 14:16
dimkl added 15 commits November 16, 2023 12:46
- `ExternalAccount.picture`
- `ExternalAccountJSON.avatar_url`
- `Organization.logoUrl`
- `OrganizationJSON.logo_url`
- `User.profileImageUrl`
- `UserJSON.profile_image_url`
- `OrganizationMembershipPublicUserData.profileImageUrl`
- `OrganizationMembershipPublicUserDataJSON.profile_image_url`
The `as string` silents the typescript error but in the runtime
the code fails if the value of `null` or `undefined` is passed
in a function that expects a string to be passed.
Failures may be caused by invoking string specific methods (eg `startsWith`)
which raise a `Cannot read properties of undefined` error.
@dimkl dimkl force-pushed the sdk-782-drop-deprecations-backend branch from 92bbeb9 to 53bef9c Compare November 16, 2023 10:47
@dimkl dimkl added this pull request to the merge queue Nov 16, 2023
Merged via the queue into main with commit cace853 Nov 16, 2023
6 checks passed
@dimkl dimkl deleted the sdk-782-drop-deprecations-backend branch November 16, 2023 11:41
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.

3 participants