fix: improve registry script DX and type safety#647
Conversation
- Make all schema fields Partial in RegistryScriptInput so composables don't require config when provided via runtime config / env vars - Drop CanBypassOptions type param (no longer needed) - Fix merge order: use defu instead of Object.assign so composable args take priority over runtime config - Add false to NuxtConfigScriptRegistryEntry for explicit disable - Skip false entries in module registry processing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a guard to skip registry entries explicitly set to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ 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.
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 (1)
src/module.ts (1)
505-512:⚠️ Potential issue | 🟡 MinorAdd an explicit check to skip
falseregistry entries intemplatePlugin.The function at src/templates.ts:121 iterates over
config.registrywithout filtering disabled entries. When a registry entry value isfalse, it falls into theelseblock (line 149) and generates initialization code with empty args (const xyz = useScriptXyz({})). Add a check after line 123 to skip entries wherec === falseto prevent generating unnecessary code for disabled scripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.ts` around lines 505 - 512, templatePlugin iterates config.registry and treats entries with value false as enabled, producing empty initialization calls; modify templatePlugin to skip registry entries whose value is strictly false (check the iterated value `c === false`) before the existing handling so disabled scripts are not emitted (i.e., do not enter the else/init generation for that entry). Ensure you reference the `templatePlugin` function and the `config.registry` iteration and skip when `c === false`.
🤖 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/runtime/types.ts`:
- Around line 220-228: Indent the type declaration beginning with "=
Partial<InferIfSchema<T>>" by two spaces to satisfy ESLint; update the leading
whitespace so the "=" line and the following "& {" block are aligned with the
surrounding type definition, and ensure the property lines (key, scriptInput,
scriptOptions) keep their current relative indentation (two spaces inside the
block) so the whole `Partial<InferIfSchema<T>> & { ... }` declaration is
consistently indented in src/runtime/types.ts.
---
Outside diff comments:
In `@src/module.ts`:
- Around line 505-512: templatePlugin iterates config.registry and treats
entries with value false as enabled, producing empty initialization calls;
modify templatePlugin to skip registry entries whose value is strictly false
(check the iterated value `c === false`) before the existing handling so
disabled scripts are not emitted (i.e., do not enter the else/init generation
for that entry). Ensure you reference the `templatePlugin` function and the
`config.registry` iteration and skip when `c === false`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fb12700-f29b-40e4-9289-c688aac02e34
📒 Files selected for processing (19)
src/module.tssrc/runtime/registry/bluesky-embed.tssrc/runtime/registry/crisp.tssrc/runtime/registry/fathom-analytics.tssrc/runtime/registry/google-adsense.tssrc/runtime/registry/hotjar.tssrc/runtime/registry/instagram-embed.tssrc/runtime/registry/intercom.tssrc/runtime/registry/matomo-analytics.tssrc/runtime/registry/meta-pixel.tssrc/runtime/registry/npm.tssrc/runtime/registry/reddit-pixel.tssrc/runtime/registry/snapchat-pixel.tssrc/runtime/registry/tiktok-pixel.tssrc/runtime/registry/vercel-analytics.tssrc/runtime/registry/x-embed.tssrc/runtime/registry/x-pixel.tssrc/runtime/types.tssrc/runtime/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/runtime/types.ts (1)
215-222: Extract conditional type to a helper for clarity.The
scriptOptionsconditional type is verbose and difficult to parse visually. Extract it to a dedicated helper type with explicitneverbranches to improve readability and make the intent clearer.♻️ Proposed refactor
+type RegistryScriptOptionOmittedKeys<Bundelable extends boolean, Usable extends boolean> = + (Bundelable extends true ? never : 'bundle') + | (Usable extends true ? never : 'use') + export interface RegistryScriptInputExtras<Bundelable extends boolean = true, Usable extends boolean = false> { /** * A unique key to use for the script, this can be used to load multiple of the same script with different options. */ key?: string scriptInput?: ScriptInput - scriptOptions?: Omit<NuxtUseScriptOptions, Bundelable extends true ? '' : 'bundle' | Usable extends true ? '' : 'use'> + scriptOptions?: Omit<NuxtUseScriptOptions, RegistryScriptOptionOmittedKeys<Bundelable, Usable>> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/types.ts` around lines 215 - 222, The conditional in RegistryScriptInputExtras's scriptOptions (type parameters Bundelable and Usable) is hard to read; extract that logic into a small named helper type (e.g., ScriptOptionsFor<Bundelable,Usable>) with explicit branches that use Omit<NuxtUseScriptOptions, 'bundle' | 'use'> and never where appropriate, then replace the inline conditional in RegistryScriptInputExtras.scriptOptions with that helper type to improve clarity while preserving the same semantics; reference the existing type name RegistryScriptInputExtras and the type parameters Bundelable and Usable when implementing the helper.
🤖 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/runtime/types.ts`:
- Line 199: The template code is still treating entries typed as
NuxtConfigScriptRegistryEntry<T> with the `false` variant as if they were
present (initializing them to {}), violating the “explicitly disable script”
semantics; locate the initialization logic in src/templates.ts that maps or
reduces the script registry (the function that builds/initializes the script
registry entries—look for names like initScriptRegistry, buildScriptEntries, or
the loop that handles registry values) and change it to skip any entry whose
value is strictly === false (do not create an empty object or default entry), so
disabled entries are omitted from the emitted template while leaving handling
for true, 'mock', T, and [T, NuxtUseScriptOptionsSerializable] unchanged.
---
Nitpick comments:
In `@src/runtime/types.ts`:
- Around line 215-222: The conditional in RegistryScriptInputExtras's
scriptOptions (type parameters Bundelable and Usable) is hard to read; extract
that logic into a small named helper type (e.g.,
ScriptOptionsFor<Bundelable,Usable>) with explicit branches that use
Omit<NuxtUseScriptOptions, 'bundle' | 'use'> and never where appropriate, then
replace the inline conditional in RegistryScriptInputExtras.scriptOptions with
that helper type to improve clarity while preserving the same semantics;
reference the existing type name RegistryScriptInputExtras and the type
parameters Bundelable and Usable when implementing the helper.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f043e3c-074b-49f2-baa9-4a2e4abdf32b
📒 Files selected for processing (1)
src/runtime/types.ts
1 participant
🔗 Linked issue
Related to #292
❓ Type of change
📚 Description
Registry script composables (
useScriptCrisp, etc.) required config options likeideven when provided via runtime config / env vars, forcingid: ''as a workaround. Additionally,Object.assigninuseRegistryScriptcaused runtime config to silently overwrite composable args — the wrong merge direction.This PR makes all schema fields
PartialinRegistryScriptInput(runtime valibot validation still catches missing fields in dev), fixes the merge order withdefuso composable args take priority, addsfalsesupport to registry config for explicitly disabling scripts, and skips disabled entries in template plugin code generation.Before:
After: