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

chore(nextjs): Apply deprecation warnings #1760

Merged
merged 6 commits into from
Sep 28, 2023
Merged

chore(nextjs): Apply deprecation warnings #1760

merged 6 commits into from
Sep 28, 2023

Conversation

dimkl
Copy link
Member

@dimkl dimkl commented Sep 21, 2023

Description

** REVIEW IT PER COMMIT **

Warn about deprecations of the following in Clerk Nextjs:

  • withClerkMiddleware
  • withServerSideAuth
  • CLERK_JS_VERSION environment variable
  • CLERK_API_KEY environment variable
  • NEXT_PUBLIC_FRONTEND_API environment variable

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 requested a review from a team as a code owner September 21, 2023 18:24
@dimkl dimkl self-assigned this Sep 21, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2023 •
edited

🦋 Changeset detected

Latest commit: 4293442

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

This PR includes changesets to release 1 package
Name Type
@clerk/nextjs 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 changed the base branch from main to drop-exported-nextjs-constants September 21, 2023 18:40
@dimkl dimkl changed the base branch from drop-exported-nextjs-constants to main September 21, 2023 18:42
@dimkl dimkl force-pushed the apply-deprecation-warnings-nextjs branch from b9a21ca to 8109799 Compare September 21, 2023 18:49
@dimkl dimkl changed the base branch from main to drop-exported-nextjs-constants September 21, 2023 18:50
LekoArts
LekoArts requested changes Sep 22, 2023
View reviewed changes
packages/nextjs/src/shared/deprecated.ts Outdated
@@ -0,0 +1,63 @@
// copy of packages/shared/src/utils/deprecated.ts
Copy link
Member

@LekoArts LekoArts Sep 22, 2023 •
edited

Choose a reason for hiding this comment

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

We need to spend time on fixing this error and the copy/paste should only be the very last resort.

What happens if you put "use client" into the usePagesOrInfinite file?

And if that doesn't help (and the file is not actually used) we need to think about splitting the utils into their own nested exports so that the bundler can tree-shake the rest

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to avoid introducing changes in @clerk/shared that would require extensive testing in this PR so I followed the naive approach of copy / paste to proceed with this PR objective and introduce another PR to support importing @clerk/shared in server-side frameworks.
Based on your comment, if we consider blocking this PR until the @clerk/shared is fixed and used I have opened the following PR in which we should also check if the @clerk/shared is tree-shakeable or not before we merge it: #1767
cc: @SokratisVidros

SokratisVidros
SokratisVidros approved these changes Sep 22, 2023
View reviewed changes
packages/nextjs/src/shared/url.ts Outdated Show resolved Hide resolved
@dimkl dimkl force-pushed the apply-deprecation-warnings-nextjs branch from 8109799 to 94cc434 Compare September 22, 2023 09:47
@dimkl dimkl changed the base branch from drop-exported-nextjs-constants to main September 22, 2023 09:47
@dimkl dimkl force-pushed the apply-deprecation-warnings-nextjs branch from 94cc434 to 2b270f1 Compare September 22, 2023 09:51
@dimkl dimkl removed the remix label Sep 22, 2023
@dimkl dimkl mentioned this pull request Sep 22, 2023
Merged
24 tasks
@dimkl dimkl changed the title chore(nextjs): Apply deprecation warnings for nextjs chore(nextjs): Apply deprecation warnings Sep 25, 2023
@dimkl dimkl requested a review from LekoArts September 27, 2023 13:08
@dimkl dimkl force-pushed the apply-deprecation-warnings-nextjs branch 2 times, most recently from 91930e0 to 333ea4d Compare September 27, 2023 15:16
@dimkl dimkl changed the base branch from main to fix-shared-deps-in-nextjs September 27, 2023 15:45
@dimkl dimkl force-pushed the apply-deprecation-warnings-nextjs branch from 333ea4d to 1f7dbe0 Compare September 28, 2023 08:27
@dimkl dimkl force-pushed the apply-deprecation-warnings-nextjs branch from 1f7dbe0 to 4293442 Compare September 28, 2023 10:54
@dimkl dimkl merged commit 921892f into fix-shared-deps-in-nextjs Sep 28, 2023
1 check passed
@dimkl dimkl deleted the apply-deprecation-warnings-nextjs branch September 28, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SokratisVidros SokratisVidros SokratisVidros approved these changes

@LekoArts LekoArts Awaiting requested review from LekoArts

Assignees

@dimkl dimkl

Labels
nextjs v5-pre-release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dimkl @SokratisVidros @LekoArts @clerk-cookie