-
Notifications
You must be signed in to change notification settings - Fork 405
chore(*): Use types from shared package
#7191
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(*): Use types from shared package
#7191
Conversation
🦋 Changeset detectedLatest commit: 9038cc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request systematically migrates type imports across 100+ files from the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes This is a high-volume but highly homogeneous refactoring. Nearly every change follows an identical pattern: replacing an import source. The mechanical nature and consistency across files reduce cognitive load. Recommended focus areas:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/expo/src/hooks/useSignInWithApple.ios.ts (1)
6-8: Missing import forSignUpUnsafeMetadata.The type
SignUpUnsafeMetadatais referenced on line 7 but is not imported. This will cause a TypeScript compilation error.Apply this diff to add the missing import:
-import type { SetActive, SignInResource, SignUpResource } from '@clerk/shared/types'; +import type { SetActive, SignInResource, SignUpResource, SignUpUnsafeMetadata } from '@clerk/shared/types';packages/expo/src/hooks/useSignInWithApple.ts (1)
5-7: Missing import forSignUpUnsafeMetadata.The type
SignUpUnsafeMetadatais referenced on line 6 but is not imported. This will cause a TypeScript compilation error.Apply this diff to add the missing import:
-import type { SetActive, SignInResource, SignUpResource } from '@clerk/shared/types'; +import type { SetActive, SignInResource, SignUpResource, SignUpUnsafeMetadata } from '@clerk/shared/types';
🧹 Nitpick comments (1)
packages/tanstack-react-start/src/server/types.ts (1)
3-10: Consider using type-only importSince these imports are only used for type composition, consider using
import typeto make the intent explicit and improve tree-shaking.Apply this diff:
-import type { +import type {Wait, looking more carefully - this already uses
import typeon line 3. Let me reconsider...Actually, the import already uses
import typesyntax. The change looks correct.LGTM: Type import migration
The migration of these type imports to
@clerk/shared/typesis correct and maintains proper type-only import syntax.
| .addScript('serve', 'pnpm preview') | ||
| .addDependency('@clerk/astro', linkPackage('astro')) | ||
| .addDependency('@clerk/types', linkPackage('types')) | ||
| .addDependency('@clerk/shared', linkPackage('types')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify package name and link path consistency
There's a potential mismatch: the package name is @clerk/shared but linkPackage('types') is called, which would resolve to packages/types directory. Based on the PR's goal to migrate from @clerk/types to @clerk/shared, should this be linkPackage('shared') instead?
Run this script to verify the correct package directory:
🏁 Script executed:
#!/bin/bash
# Check if packages/shared or packages/types directory exists and contains the expected types
echo "Checking package directories..."
if [ -d "packages/shared" ]; then
echo "✓ packages/shared exists"
ls -la packages/shared/package.json 2>/dev/null && echo " - has package.json"
fi
if [ -d "packages/types" ]; then
echo "✓ packages/types exists"
ls -la packages/types/package.json 2>/dev/null && echo " - has package.json"
fi
echo -e "\nChecking package names..."
[ -f "packages/shared/package.json" ] && echo "packages/shared name:" && cat packages/shared/package.json | jq -r '.name'
[ -f "packages/types/package.json" ] && echo "packages/types name:" && cat packages/types/package.json | jq -r '.name'Length of output: 374
The web search didn't provide specific information about the Clerk preset structure. Let me check the actual code in the repository to understand the context better.
🏁 Script executed:
#!/bin/bash
# Examine the astro.ts preset and compare with other presets
echo "=== integration/presets/astro.ts ==="
cat integration/presets/astro.ts 2>/dev/null || echo "File not found"
echo -e "\n=== Listing other presets ==="
fd -t f '\.ts$' integration/presets/ 2>/dev/null | head -10
echo -e "\n=== Checking next.ts for reference pattern ==="
[ -f "integration/presets/next.ts" ] && grep -n "addDependency.*@clerk" integration/presets/next.ts 2>/dev/null | head -10Length of output: 1428
Fix inconsistent package linkage in astro preset
The dependency linkage is incorrect. Line 14 uses linkPackage('types') which resolves to packages/types (@clerk/types), but should use linkPackage('shared') to match the package name @clerk/shared. Compare with integration/presets/next.ts line 18, which correctly uses linkPackage('shared') for the same dependency.
Change:
.addDependency('@clerk/shared', linkPackage('types'))
To:
.addDependency('@clerk/shared', linkPackage('shared'))
🤖 Prompt for AI Agents
In integration/presets/astro.ts around line 14, the preset incorrectly links
@clerk/shared to linkPackage('types') (resolving to packages/types); change the
dependency linkage to use linkPackage('shared') so @clerk/shared points to
packages/shared, matching the usage in integration/presets/next.ts.
…-package # Conflicts: # integration/presets/astro.ts # integration/templates/next-app-router/src/app/(reverification)/actions.ts # integration/types.d.ts # packages/astro/src/server/clerk-middleware.ts # packages/backend/src/tokens/authenticateContext.ts # packages/expo/src/hooks/useOAuth.ts # packages/expo/src/hooks/useSSO.ts # packages/expo/src/local-credentials/useLocalCredentials/useLocalCredentials.ts # packages/expo/src/provider/ClerkProvider.tsx # packages/nextjs/src/app-router/server/controlComponents.tsx # packages/nextjs/src/client-boundary/PromisifiedAuthProvider.tsx # packages/nextjs/src/client-boundary/hooks/useEnforceCatchAllRoute.tsx # packages/nextjs/src/client-boundary/hooks/useEnforceRoutingProps.tsx # packages/nextjs/src/experimental.ts # packages/nextjs/src/server/createGetAuth.ts # packages/nextjs/src/types.ts # packages/react-router/src/client/types.ts # packages/react-router/src/client/uiComponents.tsx # packages/tanstack-react-start/src/client/types.ts # packages/tanstack-react-start/src/client/uiComponents.tsx
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit