chore(repo): Add logic to prevent duplication of legacy hooks Typedoc#8074
chore(repo): Add logic to prevent duplication of legacy hooks Typedoc#8074SarahSoutoul wants to merge 9 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 18d5c01 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.
|
SarahSoutoul
changed the title
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@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: |
📝 WalkthroughWalkthroughAdds legacy-hook handling to the TypeDoc extraction script (.typedoc/extract-returns-and-params.mjs): introduces 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.typedoc/extract-returns-and-params.mjs:
- Around line 40-45: Extract the duplicated legacy hook detection and
filename/path rewriting into a single helper (e.g., isLegacyHook(fileName) and
rewriteLegacyOutput(fileName, dirName) or a single getLegacyOutput(fileName,
dirName)) and use it from extractReturnsSection, extractParametersSection and
moveLegacyHookDoc; the helper should encapsulate the logic that currently checks
fileName === 'use-sign-in-1' || fileName === 'use-sign-up-1', produces the
outputBaseName by stripping /-1$/ and appends '-return', and returns the new
outputDir (path.join(dirName, 'legacy')) and ensures fs.mkdirSync(..., {
recursive: true }) is called once; update those three call sites to use the
helper instead of duplicating the checks and rewrites.
- Around line 126-129: Update the JSDoc for the function that handles legacy
hook doc moves (the block currently stating "Renames legacy hook docs to use a
-legacy suffix" with `@param` {string} filePath) to accurately describe what the
code does: it moves files into a legacy/ directory and strips a trailing "-1"
from filenames rather than appending a "-legacy" suffix; ensure the description
and any examples reflect "move into legacy/ and remove -1" so future maintainers
won't be misled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76db7019-d139-4c33-8890-18be580056e3
📒 Files selected for processing (1)
.typedoc/extract-returns-and-params.mjs
|
awesome fix! but for the current useSignIn and useSignUp hooks, they're not returning the output we'd actually want in the docs: we'd want the signupsignalvalue type to be a table of those returns: |
@alexisintech Yeah that was always the case as they're not JSDoc documented properly, and currently the docs don't use these (they're manually typed). I think I'd prefer to keep the scope of this PR to just fixing the duplication, and can create another PR to make sure the output is good for those if that's okay. |
SarahSoutoul
changed the title
SarahSoutoul
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.typedoc/extract-returns-and-params.mjs:
- Around line 57-65: When legacyTarget is set the script moves outputs to
legacy/ but does not remove any existing stale files with the old "-1" suffix,
leaving duplicates (e.g. use-sign-in-1-return.mdx); update the logic around
outputBaseName/outputDir/newFilePath to detect and delete those stale artifacts
before writing: when legacyTarget is true compute the originalLegacyPath(s) (the
prior filePath + `${legacyTarget.baseName}-1-return.mdx` and `-1-params.mdx` or
any `*-1-*.mdx` for that base name) and remove them (fs.unlinkSync or fs.rmSync
with exists checks) and/or remove any `*-1-*.mdx` in the original outputDir,
then continue to create the newFilePath in legacyTarget.outputDir as now. Ensure
this cleanup runs only when legacyTarget is truthy to avoid accidental
deletions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9713ab3-ceca-4322-8997-139cddc57fdc
📒 Files selected for processing (1)
.typedoc/extract-returns-and-params.mjs
ohhh the current docs don't use them - okay sounds good! |
wobsoriano
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/short-apes-joke.md:
- Around line 1-2: The changeset file ".changeset/short-apes-joke.md" is empty
(only '---' delimiters); update it to declare the release intent by replacing
the empty frontmatter with a YAML changeset that lists the packages to bump
(each as package-name@version or package-name: patch/minor/major) and a short
summary message describing the change; ensure the file has the required opening
and closing '---' surrounding the package bump entries and the summary so
release/changelog tooling can parse it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca16d884-5dae-485a-8ece-ad878dbac729
📒 Files selected for processing (1)
.changeset/short-apes-joke.md
|
Thanks for the review @NWylynko 🙏 I can't merge this atm, but would be good to have your feedback/review too @manovotny whenever you've got the time! |
… extract script Read file content and compute legacy target once per file in the main loop instead of redundantly in each function. Also add test coverage for the new react/legacy nested folder. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
manovotny
left a comment
manovotny
left a comment
There was a problem hiding this comment.
I pushed one commit. If you hate it, we can revert it. f9f3c3f
- Small refactor to the extract script: moved the
fs.readFileSyncandgetLegacyHookTargetcalls out of the individual functions and into themain()loop so each file is read once and the legacy target is computed once, rather than redundantly inextractReturnsSection,extractParametersSection, andmoveLegacyHookDoc. All three functions now acceptcontentandlegacyTargetas parameters. - Also added test coverage for the new
react/legacynested directory and fixed the existing "top-level folders" test to filter out nested paths (it would have broken oncereact/legacyappeared).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.typedoc/extract-returns-and-params.mjs (1)
56-64:⚠️ Potential issue | 🟠 MajorDelete stale
*-1-*generated files when rerouting legacy outputs.The new files are written under
legacy/, but reruns never remove the previous top-leveluse-sign-in-1-return.mdx/use-sign-in-1-params.mdx. SincegetAllMdxFiles()skips generated files entirely, those stale artifacts will persist in any non-clean workspace and reintroduce the duplication this refactor is trying to eliminate.💡 Proposed fix
if (legacyTarget) { + const staleReturnPath = path.join(path.dirname(filePath), `${fileName}-return.mdx`); + if (fs.existsSync(staleReturnPath)) { + fs.unlinkSync(staleReturnPath); + } outputBaseName = `${legacyTarget.baseName}-return`; outputDir = legacyTarget.outputDir; fs.mkdirSync(outputDir, { recursive: true }); } @@ if (legacyTarget) { + const staleParamsPath = path.join(dirName, `${fileName}-params.mdx`); + if (fs.existsSync(staleParamsPath)) { + fs.unlinkSync(staleParamsPath); + } outputDir = legacyTarget.outputDir; outputBaseName = legacyTarget.baseName; fs.mkdirSync(outputDir, { recursive: true }); }Also applies to: 100-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.typedoc/extract-returns-and-params.mjs around lines 56 - 64, Before rerouting into legacyTarget, capture the original target paths (the pre-legacy outputBaseName and outputDir computed from fileName/filePath) and, when legacyTarget is truthy, remove any stale top-level generated files that would have been created previously (e.g. the old `${fileName}-return.mdx` and `${fileName}-params.mdx` or any `*-1-*` variants) before setting outputBaseName/outputDir and creating the new legacy file; use the same variables from the diff (outputBaseName, outputDir, legacyTarget, newFilePath) to locate and unlink those stale files so getAllMdxFiles won’t skip lingering artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.typedoc/__tests__/file-structure.test.ts:
- Around line 29-30: The isTopLevelPath function incorrectly assumes forward
slashes; normalize the filePath before checking by converting platform-specific
separators to a consistent form (e.g., replace backslashes with forward slashes
or use path.posix/path.normalize) so that nested folders from scanDirectory
(which uses path.relative/path.join) are correctly classified; update the
isTopLevelPath implementation to normalize filePath then perform the
includes('/') check and adjust any tests expecting forward-slash snapshots
accordingly.
---
Duplicate comments:
In @.typedoc/extract-returns-and-params.mjs:
- Around line 56-64: Before rerouting into legacyTarget, capture the original
target paths (the pre-legacy outputBaseName and outputDir computed from
fileName/filePath) and, when legacyTarget is truthy, remove any stale top-level
generated files that would have been created previously (e.g. the old
`${fileName}-return.mdx` and `${fileName}-params.mdx` or any `*-1-*` variants)
before setting outputBaseName/outputDir and creating the new legacy file; use
the same variables from the diff (outputBaseName, outputDir, legacyTarget,
newFilePath) to locate and unlink those stale files so getAllMdxFiles won’t skip
lingering artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50242401-f561-4e63-a943-594c04712190
📒 Files selected for processing (2)
.typedoc/__tests__/file-structure.test.ts.typedoc/extract-returns-and-params.mjs
| function isTopLevelPath(filePath: string) { | ||
| return !filePath.includes('/'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, do path.relative()andpath.join()return platform-specific separators, and would a nested path be emitted asreact\legacyon Windows instead ofreact/legacy?
💡 Result:
Yes—path.join() and path.relative() are platform-aware and return paths using the platform’s preferred separator.
path.join()“joins … using the platform-specific separator” and normalizes the result. (nodejs.org)path.relative()returns a relative path using platform conventions; the docs’ Windows example returns backslashes (e.g...\\..\\impl\\bbb). (nodejs.org)- On Windows, Node’s
pathmethods only add backslashes (\) (even though Windows accepts/too). (nodejs.org)
So on Windows a nested path would be produced as react\legacy. If you see react\\legacy, that’s typically just the JavaScript string escaping when printed/serialized (the actual string contains single backslashes). If you need forward slashes regardless of OS, use path.posix.* APIs. (nodejs.org)
Citations:
- 1: https://nodejs.org/download/nightly/v26.0.0-nightly2026022776215dc993/docs/api/path.html
- 2: https://nodejs.org/download/nightly/v26.0.0-nightly2026022776215dc993/docs/api/path.html
- 3: https://nodejs.org/download/nightly/v26.0.0-nightly2026022776215dc993/docs/api/path.html
- 4: https://nodejs.org/download/release/v22.13.0/docs/api/path.html
🏁 Script executed:
# First, locate and examine the test file
find . -name "file-structure.test.ts" -type fRepository: clerk/javascript
Length of output: 103
🏁 Script executed:
# Read the relevant sections of the test file
wc -l .typedoc/__tests__/file-structure.test.tsRepository: clerk/javascript
Length of output: 104
🏁 Script executed:
# Examine lines around 29-56 where the issue is flagged
sed -n '20,65p' .typedoc/__tests__/file-structure.test.tsRepository: clerk/javascript
Length of output: 1240
🏁 Script executed:
# Find scanDirectory() implementation
rg -n "function scanDirectory|const scanDirectory|export.*scanDirectory" --type tsRepository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Search more broadly for scanDirectory
rg -n "scanDirectory" .typedoc/__tests__/file-structure.test.ts -A 15Repository: clerk/javascript
Length of output: 1987
🏁 Script executed:
# Check the beginning of the test file to see imports and scanDirectory definition
head -30 .typedoc/__tests__/file-structure.test.tsRepository: clerk/javascript
Length of output: 985
🏁 Script executed:
# Look for where scanDirectory is defined - might be in same file or imported
rg -n "function scanDirectory|const scanDirectory" --type tsRepository: clerk/javascript
Length of output: 42
Normalize separators before classifying nested folders.
scanDirectory() uses path.relative() and path.join() (line 17), which return platform-specific separators. On Windows, nested paths become react\legacy instead of react/legacy. The current check !filePath.includes('/') will misclassify them as top-level, and the snapshot expectations with forward slashes will never match.
Proposed fix
+function normalizePath(filePath: string) {
+ return filePath.replace(/\\/g, '/');
+}
+
function isTopLevelPath(filePath: string) {
- return !filePath.includes('/');
+ return !normalizePath(filePath).includes('/');
}
@@
it('should only have these top-level folders', async () => {
const folders = await scanDirectory('directory');
- const topLevelFolders = folders.filter(isTopLevelPath);
+ const topLevelFolders = folders.map(normalizePath).filter(isTopLevelPath);
@@
it('should only have these nested folders', async () => {
const folders = await scanDirectory('directory');
- const nestedFolders = folders.filter(folder => !isTopLevelPath(folder));
+ const nestedFolders = folders.map(normalizePath).filter(folder => !isTopLevelPath(folder));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.typedoc/__tests__/file-structure.test.ts around lines 29 - 30, The
isTopLevelPath function incorrectly assumes forward slashes; normalize the
filePath before checking by converting platform-specific separators to a
consistent form (e.g., replace backslashes with forward slashes or use
path.posix/path.normalize) so that nested folders from scanDirectory (which uses
path.relative/path.join) are correctly classified; update the isTopLevelPath
implementation to normalize filePath then perform the includes('/') check and
adjust any tests expecting forward-slash snapshots accordingly.
There was a problem hiding this comment.
@manovotny just tagging you on this to see if it's sth we need to fix?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai no, you clanker! The pre-existing tests in this file already use path.relative/path.join with forward-slash snapshot assertions (see the top-level folders test). If Windows separators were a real problem here, every test in this file would already be failing. This PR just follows the established pattern, normalizing separators across the whole file would be a separate concern.
There was a problem hiding this comment.
@manovotny, you're absolutely right, and I apologize for the noise. The pre-existing top-level folders test already relies on the same path.relative/path.join calls with forward-slash snapshot assertions — if Windows separators were a real issue, those tests would have been failing long before this PR. The new nested folders test correctly follows that established pattern. I'll withdraw my concern.
5 participants
Description
The whole reason for this PR is explained in this Slack comment/ thread. We introduced the extract script to split out Returns/Parameters into standalone Typedoc files. That was working when the legacy
useSignIn/useSignUphooks were the only ones exposed. Now that the newuseSignIn/useSignUphooks are no longer@internal,Typedoc generates outputs for both the new and legacy hooks. This causes:use-sign-in.mdx,use-sign-up.mdx,use-sign-in-return.mdxetc.)use-sign-in-1*.mdx/use-sign-up-1*.mdxTo prevent this duplication, here are the changes I applied.
Changes
The extractor now detects the legacy hook outputs and:
.typedoc/docs/react/legacy/-1suffixLegacy docs are now written to:
.typedoc/docs/react/legacy/use-sign-in.mdx.typedoc/docs/react/legacy/use-sign-up.mdxLegacy returns/params are now written to:
The new signal‑based hooks keep the normal canonical paths:
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit