feat!: redesign registry config — presence = available, trigger = load#661
feat!: redesign registry config — presence = available, trigger = load#661
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
BREAKING CHANGE: Registry config no longer auto-loads scripts globally.
Adding a script to `scripts.registry` now only sets up infrastructure
(proxy routes, types, bundling, composable auto-imports). Scripts only
auto-load globally when `trigger` is explicitly set.
- `true` removed: use `{}` (infra only) or `{ trigger: 'onNuxtReady' }` (global load)
- `'proxy-only'` removed: infra-only is now the default behavior
- `trigger` hoisted to top-level config alongside script input options
- Tuple form `[input, scriptOptions]` still supported internally
Migration:
- `googleAnalytics: true` → `googleAnalytics: {}` or `googleAnalytics: { trigger: 'onNuxtReady' }`
- `googleAnalytics: { id: 'G-xxx' }` → unchanged (but no longer auto-loads; add `trigger` to auto-load)
- `googleAnalytics: 'proxy-only'` → `googleAnalytics: {}`
harlan-zw
force-pushed
the
feat/registry-config-redesign
branch
from
e1ee76a to
85c19a4
Compare
📝 WalkthroughWalkthroughThe PR changes the scripts registry shape from boolean ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/fixtures/first-party/nuxt.config.ts (1)
38-38:⚠️ Potential issue | 🟡 MinorInconsistent
vercelAnalyticsvalue in runtimeConfig.Line 38 in
runtimeConfig.public.scriptshasvercelAnalytics: true, buttrueis no longer a valid configuration value per this PR. This should be changed tovercelAnalytics: {}for consistency with the registry entry at line 76.🔧 Proposed fix
- vercelAnalytics: true, + vercelAnalytics: {},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/first-party/nuxt.config.ts` at line 38, Update the runtimeConfig.public.scripts entry for vercelAnalytics to use an object instead of a boolean: locate runtimeConfig.public.scripts and change the vercelAnalytics value from true to an empty object (vercelAnalytics: {}) so it matches the registry format used elsewhere (see the existing registry entry for vercelAnalytics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/fixtures/first-party/nuxt.config.ts`:
- Line 38: Update the runtimeConfig.public.scripts entry for vercelAnalytics to
use an object instead of a boolean: locate runtimeConfig.public.scripts and
change the vercelAnalytics value from true to an empty object (vercelAnalytics:
{}) so it matches the registry format used elsewhere (see the existing registry
entry for vercelAnalytics).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8ac1e4-8ecb-45b1-96e0-69df230f37fd
📒 Files selected for processing (9)
playground/nuxt.config.tssrc/module.tssrc/normalize.tssrc/runtime/types.tssrc/templates.tstest/fixtures/basic/nuxt.config.tstest/fixtures/first-party/nuxt.config.tstest/unit/auto-inject.test.tstest/unit/templates.test.ts
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 `@src/assets.ts`:
- Line 56: The expression creating cleanPath uses (event.path ||
'').split('?')[0] which TypeScript flags as possibly undefined; update the
assignment in src/assets.ts (the cleanPath declaration) to provide a safe
fallback for the split result before calling .slice(1) — e.g. take the first
element of the split with a nullish/empty-string fallback (so the index access
cannot be undefined) or use destructuring with a default, ensuring the final
value passed to .slice is always a string derived from event.path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
src/assets.ts
Outdated
| handler: lazyEventHandler(async () => { | ||
| return eventHandler(async (event) => { | ||
| const cleanPath = event.path.split('?')[0].slice(1) | ||
| const cleanPath = (event.path || '').split('?')[0].slice(1) |
There was a problem hiding this comment.
TypeScript error: split('?')[0] may be typed as undefined.
The pipeline is failing with TS2532. While .split() always returns at least one element at runtime, TypeScript's strict array indexing (likely noUncheckedIndexedAccess) treats array[0] as potentially undefined. Add a fallback for the array access.
🔧 Proposed fix
- const cleanPath = (event.path || '').split('?')[0].slice(1)
+ const cleanPath = ((event.path ?? '').split('?')[0] ?? '').slice(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cleanPath = (event.path || '').split('?')[0].slice(1) | |
| const cleanPath = ((event.path ?? '').split('?')[0] ?? '').slice(1) |
🧰 Tools
🪛 GitHub Actions: Test
[error] 56-56: TypeScript error TS2532: Object is possibly 'undefined'. (reported during 'nuxt typecheck' in 'pnpm typecheck')
🪛 GitHub Check: test
[failure] 56-56:
Object is possibly 'undefined'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets.ts` at line 56, The expression creating cleanPath uses (event.path
|| '').split('?')[0] which TypeScript flags as possibly undefined; update the
assignment in src/assets.ts (the cleanPath declaration) to provide a safe
fallback for the split result before calling .slice(1) — e.g. take the first
element of the split with a nullish/empty-string fallback (so the index access
cannot be undefined) or use destructuring with a default, ensuring the final
value passed to .slice is always a string derived from event.path.
acdad53 to
74c2afa
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/assets.ts (1)
56-56:⚠️ Potential issue | 🔴 CriticalFix the remaining TS2532 on
cleanPathconstruction.At Line 56,
event.path!avoids only the nullable path case;split('?')[0]is still typed as possiblyundefinedwith strict indexed access, so.slice(1)keeps typecheck failing.🔧 Proposed fix
- const cleanPath = event.path!.split('?')[0].slice(1) + const cleanPath = (event.path?.split('?')[0] ?? '').slice(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/assets.ts` at line 56, The TS2532 arises because event.path can be undefined and indexed access split('?')[0] is still considered possibly undefined; replace the non-null assertion with a safe default so the expression always yields a string. Change the construction of cleanPath to use a nullish-coalescing default on event.path (e.g., (event.path ?? '') or String(event.path)) before calling split and slice so that cleanPath is calculated from a guaranteed string; update the statement that sets cleanPath accordingly (referencing cleanPath and event.path in your edit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/assets.ts`:
- Line 56: The TS2532 arises because event.path can be undefined and indexed
access split('?')[0] is still considered possibly undefined; replace the
non-null assertion with a safe default so the expression always yields a string.
Change the construction of cleanPath to use a nullish-coalescing default on
event.path (e.g., (event.path ?? '') or String(event.path)) before calling split
and slice so that cleanPath is calculated from a guaranteed string; update the
statement that sets cleanPath accordingly (referencing cleanPath and event.path
in your edit).
1 participant
Summary
Redesigns the
scripts.registryconfig API to separate "availability" from "loading":trigger= global auto-loading (opt-in, explicit)This eliminates the
proxy-onlyhack, removes the need for$productionwrappers in most cases, and simplifies the mental model.Before
After
Breaking Changes
trueremoved as config value (build error with migration message)'proxy-only'removed (build error with migration message; infra-only is now the default)triggerno longer auto-load globallytriggerhoisted to top-level of config object alongside script input options[input, scriptOptions]still supported internallyMigration
googleAnalytics: truegoogleAnalytics: {}orgoogleAnalytics: { trigger: 'onNuxtReady' }googleAnalytics: { id: 'G-xxx' }(wanted global load)googleAnalytics: { id: 'G-xxx', trigger: 'onNuxtReady' }googleAnalytics: { id: 'G-xxx' }(wanted composable only)googleAnalytics: 'proxy-only'googleAnalytics: {}$production: { scripts: { ... } }Production-only improvement
Previously required wrapping everything in
$production, which disabled types and composables in dev. Now:Test plan
{}entriestrigger: 'onNuxtReady'generates correct plugin codetrueand'proxy-only'throw clear build errors