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.

Sign up for GitHub

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

Jump to bottom

feat(nextjs,shared): Improve CLERK_API_URL default value #1955

Merged
merged 2 commits into from
Nov 3, 2023
Merged

feat(nextjs,shared): Improve CLERK_API_URL default value #1955

merged 2 commits into from
Nov 3, 2023

Conversation

desiprisg
Copy link
Contributor

@desiprisg desiprisg commented Oct 30, 2023 •
edited
Loading

Description

Improve the default value for CLERK_API_URL by utilizing the publishable key to differentiate between local, staging and prod environments.

We can also add this to other packages that can import the shared package.

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

@changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 149da8b

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

This PR includes changesets to release 11 packages
Name Type
@clerk/nextjs Patch
@clerk/remix Patch
gatsby-plugin-clerk Patch
@clerk/shared Patch
@clerk/fastify Patch
@clerk/backend Patch
@clerk/clerk-js Patch
@clerk/clerk-expo Patch
@clerk/clerk-react Patch
@clerk/clerk-sdk-node Patch
@clerk/chrome-extension 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

@desiprisg desiprisg requested a review from a team October 30, 2023 01:40
octoper
octoper approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@octoper octoper left a comment

Choose a reason for hiding this comment

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

💯

LekoArts
LekoArts requested changes Oct 30, 2023
View reviewed changes
.changeset/rude-jobs-yawn.md Outdated Show resolved Hide resolved
packages/shared/src/index.ts Show resolved Hide resolved
packages/shared/src/utils/apiUrlFromPublishableKey.ts Outdated Show resolved Hide resolved
panteliselef
panteliselef requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@panteliselef panteliselef left a comment

Choose a reason for hiding this comment

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

❓ Shouldn't we merge to main-v4 ?

packages/shared/src/utils/apiUrlFromPublishableKey.ts Outdated Show resolved Hide resolved
@dimkl
Copy link
Contributor

dimkl commented Oct 30, 2023

❓ Shouldn't we merge to main-v4 ?

@panteliselef even though this is not a breaking change we should merge it to main. This is an improvement in our current resolution of apiUrl and not a hotfix that should be deployed to main-v4.

@desiprisg
Copy link
Contributor Author

desiprisg commented Oct 31, 2023 •
edited
Loading

Not merging yet as DEV_OR_STAGING_SUFFIXES seems to mean dev as in dev instance and not local. This is why accounts.dev in included in there. I'll have to make a different suffixes array for this case only.

dimkl
dimkl suggested changes Oct 31, 2023
View reviewed changes
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.

Blocking this PR until unit test is added to avoid accidentally merging it.

LekoArts
LekoArts requested changes Nov 1, 2023
View reviewed changes
packages/clerk-js/src/utils/url.ts Outdated Show resolved Hide resolved
packages/nextjs/src/server/url.ts Outdated Show resolved Hide resolved
@desiprisg desiprisg force-pushed the improve_api_url_default branch 3 times, most recently from e887e97 to a0e4b24 Compare November 2, 2023 12:49
LekoArts
LekoArts requested changes Nov 2, 2023
View reviewed changes
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.

If you add apiUrlFromPublishableKey and constants as individual files and to the export of the main index file you'll need to add both here:

  1. https://github.com/clerkinc/javascript/blob/main/packages/shared/package.json#L47-L68
  2. https://github.com/clerkinc/javascript/blob/main/packages/shared/subpaths.mjs#L4-L23

@desiprisg desiprisg force-pushed the improve_api_url_default branch 2 times, most recently from 1526d50 to 028876c Compare November 3, 2023 10:55
@dimkl dimkl self-requested a review November 3, 2023 14:11
dimkl
dimkl reviewed Nov 3, 2023
View reviewed changes
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.

I reverted the request changes

@dimkl dimkl self-requested a review November 3, 2023 14:13
@desiprisg desiprisg added this pull request to the merge queue Nov 3, 2023
@tmilewski tmilewski removed this pull request from the merge queue due to a manual request Nov 3, 2023
@desiprisg desiprisg added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit 7644b74 Nov 3, 2023
7 checks passed
@desiprisg desiprisg deleted the improve_api_url_default branch November 3, 2023 15:32
@clerk-cookie clerk-cookie mentioned this pull request Nov 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SokratisVidros SokratisVidros SokratisVidros left review comments

@panteliselef panteliselef panteliselef approved these changes

@gkats gkats gkats approved these changes

@octoper octoper octoper approved these changes

@LekoArts LekoArts LekoArts approved these changes

@dimkl dimkl Awaiting requested review from dimkl

Assignees
No one assigned
Labels
clerk-js fastify gatsby nextjs remix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@desiprisg @dimkl @gkats @SokratisVidros @octoper @LekoArts @panteliselef @clerk-cookie