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

fix(clerk-js): Remove internal useCore hooks #2111

Merged
merged 8 commits into from
Nov 28, 2023
Merged

fix(clerk-js): Remove internal useCore hooks #2111

merged 8 commits into from
Nov 28, 2023

Conversation

LekoArts
Copy link
Member

@LekoArts LekoArts commented Nov 10, 2023 •
edited by panteliselef
Loading

Description

This PR:

  • Merges useCoreSession (clerk-js) and useSession(clerk-react) and moves it to @clerk/shared/react as useSession.
  • Merges useCoreUser (clerk-js) and useUser(clerk-react) and moves it to @clerk/shared/react as useUser.
  • Merges useCoreClerk (clerk-js) and useClerk(clerk-react) and moves it to @clerk/shared/react as useClerk.
  • Renames useCoreOrganization(List) hooks to useOrganization(List) inside clerk-js.
  • Improves parity between the hooks we are exposing and the hooks we use internally.

The changes for the clerk-js package are applied inside the ui.retheme and the ui directory

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

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: ee24072

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

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

@panteliselef panteliselef force-pushed the lekoarts/sdk-660-remove-usecorex-hooks-and-instead-call-usex-directly-for-our branch 2 times, most recently from 3a08302 to 37e8513 Compare November 12, 2023 17:18
@panteliselef panteliselef marked this pull request as ready for review November 12, 2023 18:03
@panteliselef panteliselef requested a review from a team as a code owner November 12, 2023 18:03
@panteliselef panteliselef force-pushed the lekoarts/sdk-660-remove-usecorex-hooks-and-instead-call-usex-directly-for-our branch from 37e8513 to e9d23e0 Compare November 13, 2023 12:21
@panteliselef panteliselef force-pushed the lekoarts/sdk-660-remove-usecorex-hooks-and-instead-call-usex-directly-for-our branch 4 times, most recently from dde7159 to 9ad4505 Compare November 23, 2023 09:16
dimkl
dimkl approved these changes Nov 23, 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.

❓ Instead of replacing user. with user?. in the components where useUser() hook is used, shouldn't we add a guard if (!user) return null;?
❓ Instead of adding the if (!user) return null; guard could we move that up the hierarchy to avoid repeating this guard to all the components of eg UserProfile / OrganizationSwitcher? (If so, let's verify that all components that need user have the guard and add a separate PR to make that improvement)

packages/clerk-js/src/ui.retheme/components/OrganizationList/UserMembershipList.tsx

if (!user) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ is this a fix to avoid rendering PersonalAccountPreview when user is not available or the useCoreUser() has different behaviour from the useUser() ?

Copy link
Member

Choose a reason for hiding this comment

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

OrganizationList is wrapped in withCoreUserGuard so we are safe. It doesn't really matter if we do user?.something or return null.

Here we did that because it was helping with the deconstruction.

packages/clerk-js/src/ui.retheme/components/OrganizationProfile/ActionConfirmationPage.tsx

if (!organization) {
if (!organization || !user) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

packages/clerk-js/src/ui.retheme/components/OrganizationProfile/ActiveMembersList.tsx
@@ -76,9 +76,9 @@ const MemberRow = (props: {
const { membership, onRemove, onRoleChange, options } = props;
const { localizeCustomRole } = useLocalizeCustomRoles();
const card = useCardState();
const user = useCoreUser();
const { user } = useUser();
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Don't we need if (!user) return null; guard here?

Copy link
Member

Choose a reason for hiding this comment

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

OrganizationProfile is wrapped in withCoreUserGuard so we are safe.

packages/react/src/contexts/ClientContext.tsx Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

🔧 remove this empty file.

@panteliselef panteliselef force-pushed the lekoarts/sdk-660-remove-usecorex-hooks-and-instead-call-usex-directly-for-our branch 2 times, most recently from b46658d to 41fed9e Compare November 28, 2023 08:59
@panteliselef panteliselef force-pushed the lekoarts/sdk-660-remove-usecorex-hooks-and-instead-call-usex-directly-for-our branch from dc7b7a4 to ee24072 Compare November 28, 2023 18:15
@panteliselef panteliselef added this pull request to the merge queue Nov 28, 2023
Merged via the queue into main with commit 4b8bedc Nov 28, 2023
7 checks passed
@panteliselef panteliselef deleted the lekoarts/sdk-660-remove-usecorex-hooks-and-instead-call-usex-directly-for-our branch November 28, 2023 21:42
@clerk-cookie clerk-cookie mentioned this pull request Nov 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panteliselef panteliselef panteliselef left review comments

@dimkl dimkl dimkl approved these changes

@BRKalow BRKalow BRKalow approved these changes

Assignees

@LekoArts LekoArts

@panteliselef panteliselef

Labels
clerk-js react
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LekoArts @BRKalow @dimkl @panteliselef @clerk-cookie