fix(eslint-plugin): Fix initial lint rule bugs#8941
Conversation
Also changes the fallback to double quotes to match Prettier, ESLint and `create-next-app` defaults
Now recognizes `if (!isAuthenticated || otherCondition)` as valid
🦋 Changeset detectedLatest commit: 7a21eca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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.
|
📝 WalkthroughWalkthroughThree improvements to the Changesrequire-auth-protection rule improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/eslint-plugin/src/next/lib/fixers.ts`:
- Around line 135-139: The current code allows async insertion for
type-predicate return types while only skipping the return-type wrapping, which
produces invalid TypeScript. Instead of returning null to skip only the return
type modification, move the check for TSTypePredicate to an earlier point in the
addAuthProtectFixes function to prevent the entire fix from being applied to
functions with type-predicate return types. This way, when a function has a
type-predicate return annotation (detected via typeAnnotation.type ===
'TSTypePredicate'), the entire fixer should exit early rather than allowing a
partial transformation that combines async with an incompatible return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0835949b-dd74-43ac-bffe-a2efd415fa88
📒 Files selected for processing (10)
.changeset/eslint-plugin-add-promise-type.md.changeset/eslint-plugin-fixer-quote-style.md.changeset/eslint-plugin-or-auth-guards.mdpackages/eslint-plugin/src/next/__tests__/fix-auth-protection.test.tspackages/eslint-plugin/src/next/__tests__/quote-style.test.tspackages/eslint-plugin/src/next/__tests__/require-auth-protection.suggestions.test.tspackages/eslint-plugin/src/next/__tests__/require-auth-protection.test.tspackages/eslint-plugin/src/next/lib/fixers.tspackages/eslint-plugin/src/next/lib/protection-checks.tspackages/eslint-plugin/src/next/lib/quote-style.ts
| import { auth } from '@clerk/nextjs/server'; | ||
| export default async function Page() { | ||
| const { isAuthenticated } = await auth(); | ||
| if (!isAuthenticated || someFlag) return null; |
There was a problem hiding this comment.
Directional question.. should we allow mix of auth/not-auth conditional logic here or should it be strictly auth.* || auth.* ?
There was a problem hiding this comment.
This is a good question and I asked myself the same, but I don't see why not? As long as it always happens for !isAuthenticated I think it's fine.
There is a small risk of if (!isAuthenticated || doDangerousThing()), but I feel that should be rare and obvious enough that DX wins here. We already allow arbitrary code above the early exit now which is not exactly the same (we are already in an "unauthed state" there, which we are not when evaluating the if), but close enough?
jacekradko
left a comment
jacekradko
left a comment
There was a problem hiding this comment.
Looks good. Just an exploratory question on logical checks
2 participants
Description
I bundled 3 small fixes into this PR, I recommend reviewing per commit.
Promise<>when making the functionasyncfunction Component(): React.ReactElementwould becomeasync function Component(): JSX.Elementwhich would fail TSfunction Component(): Promise<React.ReactElement>type Return = Promise<...>;can get double-wrapped viaPromise<Return>- we can't follow it, but this should be very rare and fine in practice, worst case TS complains and you have to fix it manuallyimport { auth } from "@clerk/nextjs/server"match the quote style of already existing imports"to match Prettier, ESLint andcreate-next-appdefaults||in custom checks, likeif (!isAuthenticated || !has(...))Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
require-auth-protectionrule now recognizes authentication guards expressed with OR-conditions (e.g.,if (!isAuthenticated || otherCondition)).Promise<>when converting functions toasync.