feat(globals): runtime disable + scripts:globals hook#808
Merged
Conversation
Globals are resolved at build time, so a single build couldn't drop or recompute an entry per instance. Two runtime escapes, both keyed off the existing scriptsGlobals override: - Registration now routes through a guard that skips an entry whose merged input has enabled: false or an empty/null src, so an unused integration can be dropped per deployment via NUXT_PUBLIC_SCRIPTS_GLOBALS_<KEY>_SRC=. - The generated plugin builds a mutable map of resolved inputs and fires a scripts:globals app hook before registering, letting userland rewrite src, delete keys, or add entries from runtime config. Static globals keep their $scripts types and asset bundling, which a globals factory would lose. Related to #759
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
📝 WalkthroughWalkthroughThis PR adds a new Nuxt App hook, scripts:globals, and updates the plugin template to collect globals into a mutable __globals map, invoke the hook before registration, and use a guarded __registerGlobal helper that skips disabled or empty-src entries. It makes generated setup async when globals exist, documents the hook and per-instance env overrides, and updates unit, e2e, and fixture tests to validate rewrite/deletion/disable semantics. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (3)
test/fixtures/issue-759/plugins/globals.ts (1)
6-6: ⚡ Quick winLet the callback infer the real
scripts:globalshook type.The explicit
Record<string, Record<string, any>>annotation masks regressions in the new hook typing this fixture is supposed to exercise. Dropping it keeps the fixture pinned to the generated Nuxt hook contract.♻️ Proposed fix
- nuxtApp.hooks.hook('scripts:globals', (globals: Record<string, Record<string, any>>) => { + nuxtApp.hooks.hook('scripts:globals', (globals) => { globals.scrads.src = 'https://scrads.example/from-hook.js' })🤖 Prompt for 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. In `@test/fixtures/issue-759/plugins/globals.ts` at line 6, Remove the explicit type annotation on the callback parameter so the hook callback can infer the real "scripts:globals" hook type; locate the nuxtApp.hooks.hook call (nuxtApp.hooks.hook('scripts:globals', (globals: Record<string, Record<string, any>>) => { ... })) and change it to use an untyped parameter (e.g., (globals) => { ... }) so the fixture exercises the generated Nuxt hook contract rather than masking it.test/e2e/issue-759-globals-env-override.test.ts (1)
33-39: ⚡ Quick winAssert the registered
$scripts.scradsvalue too.This only proves the rewritten
hrefmade it into the HTML head. It can still pass if the preload/link path updates while$scripts.scradskeeps the bakedsrc.app.vuealready exposes#scrads-src, so add an assertion for that rendered value as well.♻️ Proposed fix
it('the scripts:globals hook rewrites a global src at runtime', async () => { const html = await $fetch<string>('/') // The hook-mutated src is what actually gets registered/preloaded... expect(html).toContain('href="https://scrads.example/from-hook.js"') + expect(html).toContain('<div id="scrads-src">https://scrads.example/from-hook.js</div>') // ...while the un-mutated build-time src is never used for registration. expect(html).not.toContain('href="https://scrads.example/baked.js"') + expect(html).not.toContain('<div id="scrads-src">https://scrads.example/baked.js</div>') })🤖 Prompt for 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. In `@test/e2e/issue-759-globals-env-override.test.ts` around lines 33 - 39, Add an assertion to the test that verifies the runtime-registered $scripts.scrads value was updated by the scripts:globals hook by reading the DOM element rendered by app.vue with id "`#scrads-src`" (exposed in the app) and asserting its text/content equals "https://scrads.example/from-hook.js"; keep existing HTML head assertions but add this extra check so $scripts.scrads is validated as rewritten at runtime.test/unit/templates.test.ts (1)
342-361: ⚡ Quick winAdd coverage for a deleted/
undefinedentry.The guard unit test exercises
enabled: false, empty/null/undefinedsrc, but not the case where the entry itself isundefined(i.e. a hookdelete globals.<key>). That path currently throws on destructuring (see the critical issue intemplates.ts). After the guard fix, addexpect(__registerGlobal(undefined, {})).toBeUndefined()so the regression is locked in.🤖 Prompt for 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. In `@test/unit/templates.test.ts` around lines 342 - 361, Add a test asserting the deleted/undefined entry case by adding expect(__registerGlobal(undefined, {})).toBeUndefined() to the "__registerGlobal skips disabled entries and registers the rest" test, and ensure the implementation of __registerGlobal in templates.ts defensively handles an undefined input (e.g., early return before destructuring) so it no longer throws when called with undefined.
🤖 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/script/src/templates.ts`:
- Around line 242-255: The generated __registerGlobal currently destructures its
argument unguarded and throws when a hook deletes an entry; update
__registerGlobal (the function used to wrap useScript calls) to guard against a
missing/falsy input before destructuring (e.g. return undefined immediately if
!input or use a default param) so deleted entries from __globals don't crash
plugin setup; keep the existing checks for enabled/empty/null src and ensure
call sites in the setupBody that build __globals and globalInits remain
compatible. Also update the inline snapshot and the unit tests in
test/unit/templates.test.ts to reflect the new behavior and add a test case for
a deleted/global entry.
- Around line 248-257: The current implementation builds registrations only from
the static config.globals (globalMapEntries/globalInits) and never handles keys
added by the runtime hook after await nuxtApp.hooks.callHook('scripts:globals',
__globals); update templates.ts so that immediately after calling the hook you
re-scan Object.keys(__globals) and for any key not already handled push a
registration/init that calls __registerGlobal(__globals["<key>"], ...) (add to
globalInits or inits as appropriate) and ensure the final provide.scripts list
is derived from Object.keys(__globals) combined with resolvedRegistryKeys
instead of only Object.keys(config.globals), so runtime-added keys are
registered and exposed; retain existing behavior for statically declared keys
and document/type-note that bundlers/types remain static.
- Around line 29-31: The generated type for the 'scripts:globals' hook is
currently a closed object with required properties, causing TS2790 when
deleting/adding keys; update the template in packages/script/src/templates.ts
that builds the 'scripts:globals' signature so each mapped key is optional (e.g.
use `?:`) and include an index signature like `[key: string]: Record<string,
any>` so unknown additions and deletions type-check; adjust the globalsKeys.map
generation to emit optional properties and append the index signature to the
produced object type.
---
Nitpick comments:
In `@test/e2e/issue-759-globals-env-override.test.ts`:
- Around line 33-39: Add an assertion to the test that verifies the
runtime-registered $scripts.scrads value was updated by the scripts:globals hook
by reading the DOM element rendered by app.vue with id "`#scrads-src`" (exposed in
the app) and asserting its text/content equals
"https://scrads.example/from-hook.js"; keep existing HTML head assertions but
add this extra check so $scripts.scrads is validated as rewritten at runtime.
In `@test/fixtures/issue-759/plugins/globals.ts`:
- Line 6: Remove the explicit type annotation on the callback parameter so the
hook callback can infer the real "scripts:globals" hook type; locate the
nuxtApp.hooks.hook call (nuxtApp.hooks.hook('scripts:globals', (globals:
Record<string, Record<string, any>>) => { ... })) and change it to use an
untyped parameter (e.g., (globals) => { ... }) so the fixture exercises the
generated Nuxt hook contract rather than masking it.
In `@test/unit/templates.test.ts`:
- Around line 342-361: Add a test asserting the deleted/undefined entry case by
adding expect(__registerGlobal(undefined, {})).toBeUndefined() to the
"__registerGlobal skips disabled entries and registers the rest" test, and
ensure the implementation of __registerGlobal in templates.ts defensively
handles an undefined input (e.g., early return before destructuring) so it no
longer throws when called with undefined.
🪄 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: 2b689a21-1bad-4c68-810e-dccedb91351a
📒 Files selected for processing (8)
docs/content/docs/1.guides/4.global.mddocs/content/docs/3.api/6.nuxt-app-hooks.mdpackages/script/src/templates.tstest/e2e/issue-759-globals-env-override.test.tstest/fixtures/issue-759/app.vuetest/fixtures/issue-759/nuxt.config.tstest/fixtures/issue-759/plugins/globals.tstest/unit/templates.test.ts
Addresses CodeRabbit review on #808: - __registerGlobal guards against a missing input, so a listener that does `delete globals[key]` no longer throws 'Cannot destructure ... undefined' and abort plugin setup. Covered by a unit case and an e2e delete case. - Hook type is an index signature (Record<string, Record<string, any>>) so `delete globals[key]` and `globals[key].src = ...` both typecheck (no TS2790 / possibly-undefined). - Docs no longer claim the hook can add brand-new globals; registration only iterates declared keys, so net-new scripts go through useScript() in a plugin.
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/unit/templates.test.ts (1)
319-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the runtime-hook contract in this test comment.
The generated plugin only registers statically declared keys, so brand-new entries added to
__globalswill not be registered. Saying the hook can “rewrite/delete/add” entries here conflicts with the documented behavior and is likely to mislead future readers.🤖 Prompt for 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. In `@test/unit/templates.test.ts` around lines 319 - 322, Update the test comment for "globals are passed through the scripts:globals runtime hook before registration" to reflect the actual runtime-hook contract: the generated plugin only registers statically declared keys so the hook may rewrite or delete existing entries on __globals but cannot add brand-new keys that will be registered; reference the scripts:globals runtime hook and __globals in the comment so readers know additions won't be picked up by the static registration.
🤖 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.
Outside diff comments:
In `@test/unit/templates.test.ts`:
- Around line 319-322: Update the test comment for "globals are passed through
the scripts:globals runtime hook before registration" to reflect the actual
runtime-hook contract: the generated plugin only registers statically declared
keys so the hook may rewrite or delete existing entries on __globals but cannot
add brand-new keys that will be registered; reference the scripts:globals
runtime hook and __globals in the comment so readers know additions won't be
picked up by the static registration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2c7a00c-54fd-4307-a157-4c3dde3c8896
📒 Files selected for processing (8)
docs/content/docs/1.guides/4.global.mddocs/content/docs/3.api/6.nuxt-app-hooks.mdpackages/script/src/templates.tstest/e2e/issue-759-globals-env-override.test.tstest/fixtures/issue-759/app.vuetest/fixtures/issue-759/nuxt.config.tstest/fixtures/issue-759/plugins/globals.tstest/unit/templates.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/content/docs/1.guides/4.global.md
- test/fixtures/issue-759/app.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- test/fixtures/issue-759/plugins/globals.ts
- docs/content/docs/3.api/6.nuxt-app-hooks.md
- packages/script/src/templates.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Related to #759
❓ Type of change
📚 Description
Follow-up to #780.
scripts.globalsentries are resolved at build time, so a single build can't drop or recompute one per instance. The env override added in #780 only mutates values of existing keys, so unused integrations still register on every deployment. This adds two runtime escapes, both keyed off the existingscriptsGlobalsoverride:enabled: falseor an empty/nullsrc. A tenant that doesn't use Awin drops it withNUXT_PUBLIC_SCRIPTS_GLOBALS_AWIN_SRC=, no rebuild. The key resolves toundefinedon$scripts.scripts:globalsruntime hook. The generatedscripts:initplugin builds a mutable map of the resolved inputs and fires the hook before registering, so userland can rewritesrc, delete keys, or add entries from runtime config. Register the listener in anenforce: 'pre'plugin so it runs first.I used a hook rather than a
globals(runtimeConfig)factory because globals are resolved at build to generate the$scriptstypes and feed asset bundling; a factory at server startup happens after that and loses both. The hook keeps statically declared entries typed and bundled while letting you compute the rest at startup.Covered by unit tests (guard logic, hook ordering, generated map shape) and the
issue-759e2e (env override, disable-via-empty-src, hook rewritessrc, all against a built fixture).