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(*): Improve @clerk/clerk-react DX #2328

Merged
merged 15 commits into from
Dec 14, 2023
Merged

chore(*): Improve @clerk/clerk-react DX #2328

merged 15 commits into from
Dec 14, 2023

Conversation

dimkl
Copy link
Contributor

@dimkl dimkl commented Dec 12, 2023

Description

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 requested a review from nikosdouvlis December 12, 2023 19:41
@dimkl dimkl self-assigned this Dec 12, 2023
Copy link

changeset-bot bot commented Dec 12, 2023

🦋 Changeset detected

Latest commit: 00e342a

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

This PR includes changesets to release 6 packages
Name Type
gatsby-plugin-clerk Major
@clerk/chrome-extension Major
@clerk/nextjs Major
@clerk/clerk-react Major
@clerk/remix Major
@clerk/clerk-expo Major

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-975-react-dx branch 2 times, most recently from 79b8de6 to 69c5aa9 Compare December 13, 2023 13:10
@dimkl dimkl marked this pull request as ready for review December 13, 2023 13:11
@dimkl dimkl marked this pull request as draft December 13, 2023 13:16
.changeset/mighty-bulldogs-try.md Show resolved Hide resolved
@@ -48,5 +45,5 @@ export const WithClerk: React.FC<{
return null;
}

return <LoadedGuarantee>{children(clerk as unknown as LoadedClerk)}</LoadedGuarantee>;
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Can you please elaborate what replaces the loaded guarantee?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing used the LoadedGuarantee (or "structure context") since the v4 hook change. We kept it around in case we wanted to use it to throw better errors in cases where the Clerk hooks were used outside the Clerk provider but ended up doing that by simply checking wether the ClerkProvider context exists or not.

Extra context: The LoadedGuarantee (or "structure context") used to wrap all SignedIn and SignedOut components and the older hooks could only be used when nested under these components - this is no longer the case as all hooks return isLoaded

Copy link
Member

Choose a reason for hiding this comment

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

cc @dimkl

isEmailLinkError,
isKnownError,
isMetamaskError,
EmailLinkErrorCode,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both isEmailLinkError and EmailLinkErrorCode?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about exporting ClerkRuntimeError and ClerkAPIResponseError?

Copy link
Contributor Author

@dimkl dimkl Dec 13, 2023

Choose a reason for hiding this comment

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

Do we need both isEmailLinkError and EmailLinkErrorCode?

I think we should keep only the isEmailLinkError

How about exporting ClerkRuntimeError and ClerkAPIResponseError?

Let's keep only the helpers to allow the consumers of that package to identify and handle those errors but for now since those errors are supposed to be thrown by Clerk let's avoid exposing them.
If we need to use them in any of our packages we can import them from @clerk/shared

packages/react/src/internal.ts Outdated Show resolved Hide resolved
@dimkl dimkl marked this pull request as ready for review December 13, 2023 18:22
@dimkl dimkl closed this Dec 13, 2023
@dimkl dimkl reopened this Dec 13, 2023
@tmilewski
Copy link
Member

@dimkl Rebase main and CI should be good.

dimkl and others added 6 commits December 14, 2023 01:10
Actions:
- Use `tsc` on success to produce types
- Use `emitDeclarationOnly: true`
- Replace `d.mts` with `d.ts` types in package.json
- Move `setErrorThrowerOptions` definition from `/internal` to `errors.ts`

Why:
- Enabling `dts` was causing build issues with controlComponents
inferred type in `@clerk/remix` package
- Emitting declarations only is used to avoid tampering the bundled code
which is configured as part of tsup
- Using `d.ts` files for types is set because `d.mts` are not generated
since we disabled dts option and the generated `d.mts` are the same as
`d.ts` files based on some comparisons
- Moving the `setErrorThrowerOptions` was done to simplify the
  dependency tree
@dimkl dimkl added this pull request to the merge queue Dec 14, 2023
Merged via the queue into main with commit 8aea39c Dec 14, 2023
8 checks passed
@dimkl dimkl deleted the sdk-975-react-dx branch December 14, 2023 07:02
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.

6 participants