fix(platform): reset cached tViews between SSR requests for correct i18n locale switching#2301
Conversation
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRemoved the old Vite post-transform plugin Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
This PR touches multiple package scopes: Please confirm the changes are closely related. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/platform/src/lib/ssr/ssr-build-plugin.ts (1)
1-45: LGTM on thessrBuildPluginrefactor.Switching to
transform.filter+handlerwithid.includeregexes reduces hook invocations without changing behavior; the in-handlerid.includes(...)checks also serve as the recommended backward-compat guard for Vite < 6.3.0. The addeddomino/lib/sloppy.js→with(→if(rewrite is consistent with the other entries.One small nit you may defer:
code.replace(/with\(/gi, 'if(')uses a case-insensitive flag, which would also rewrite an accidentalWITH(inside comments/strings. Given the target file is a known fixed domino shim this is almost certainly fine — just worth a sanity check if domino is ever updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/platform/src/lib/ssr/ssr-build-plugin.ts` around lines 1 - 45, The regex in ssrBuildPlugin's transform handler currently uses a case-insensitive replace (/with\(/gi) which could accidentally rewrite occurrences like "WITH(" in comments/strings; change the replacement to only match the lowercase token (e.g., use /with\(/g) or a more specific pattern that limits matches to actual code contexts, updating the code.replace call inside the handler for id.includes(normalizePath('domino/lib/sloppy.js')) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/platform/src/lib/ssr/ssr-build-plugin.ts`:
- Around line 59-85: The i18nDefRegistryPlugin currently patches `@angular/core`
by string-matching DETECT_MARKER and RETURN_STMT inside i18nDefRegistryPlugin(),
which can silently fail if Angular's output changes; add a unit test file
(ssr-build-plugin.spec.ts) that feeds a representative `@angular/core` SSR module
string and asserts the replacement mutates the code and that
globalThis.__ngComponentDefs is added; after the replace() call verify
transformed.code !== code and if identical either throw an Error (for build) or
call this.warn/error (depending on context) to surface contract breakage; also
add apply: 'build' to the returned plugin (or a comment documenting why dev SSR
is allowed) and add a top-of-file comment pinning the supported `@angular/core`
version (e.g. 21.2.7) with a link to the getComponentId contract.
In `@packages/router/server/src/render.ts`:
- Around line 37-43: The global Set __ngComponentDefs is mutated by
resetComponentDefTViews() with no concurrency guard, which can race with
concurrent calls to renderApplication(); either document the serialization
guarantee or serialize the operations: add a clear comment near
resetComponentDefTViews() stating the runtime/Nitro SSR handler serializes
requests (so concurrent mutation cannot occur), or else wrap the sequence that
calls resetComponentDefTViews() and renderApplication() with an async
mutex/limit (e.g., pLimit or a simple Promise mutex) to ensure
resetComponentDefTViews() completes and renderApplication() runs without
interleaving; refer to resetComponentDefTViews and renderApplication in the
change.
---
Nitpick comments:
In `@packages/platform/src/lib/ssr/ssr-build-plugin.ts`:
- Around line 1-45: The regex in ssrBuildPlugin's transform handler currently
uses a case-insensitive replace (/with\(/gi) which could accidentally rewrite
occurrences like "WITH(" in comments/strings; change the replacement to only
match the lowercase token (e.g., use /with\(/g) or a more specific pattern that
limits matches to actual code contexts, updating the code.replace call inside
the handler for id.includes(normalizePath('domino/lib/sloppy.js')) accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7a61745-99db-444b-95b1-69e43d4f2021
📒 Files selected for processing (7)
packages/platform/src/lib/i18n-component-registry-plugin.tspackages/platform/src/lib/platform-plugin.tspackages/platform/src/lib/ssr/ssr-build-plugin.tspackages/router/server/src/render.tspackages/router/src/index.tspackages/router/src/lib/i18n/provide-i18n.spec.tspackages/router/src/lib/i18n/provide-i18n.ts
💤 Files with no reviewable changes (4)
- packages/router/src/index.ts
- packages/router/src/lib/i18n/provide-i18n.spec.ts
- packages/platform/src/lib/i18n-component-registry-plugin.ts
- packages/router/src/lib/i18n/provide-i18n.ts
| function resetComponentDefTViews(): void { | ||
| const defs = (globalThis as any).__ngComponentDefs as Set<any> | undefined; | ||
| if (!defs) return; | ||
| for (const def of defs) { | ||
| def.tView = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for how render() is invoked from the SSR entry / nitro handler and
# whether there is any surrounding serialization. Also confirm renderApplication
# is fully awaited before the next request can call render() again.
rg -nP -C5 '\brender\s*\(' packages/router/server/src
rg -nP -C3 'renderApplication' packages/router/server/src
fd -t f 'server.*\.(mjs|ts|js)$' -E node_modules -E dist \
--exec rg -nP -C3 "from '@analogjs/router/server'" {} \;Repository: analogjs/analog
Length of output: 6095
🏁 Script executed:
fd -t f 'handler\|entry\|middleware' packages/router -E node_modules -E dist | head -20Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
rg -nP -l 'defineEventHandler\|renderSSR\|export.*render' packages/ -E node_modules -E dist | grep -E '\.(ts|js|mjs)$' | head -20Repository: analogjs/analog
Length of output: 100
🏁 Script executed:
fd -t f '\.(ts|js|mjs)$' apps/ -E node_modules -E dist -E '.next' | xargs rg -l 'renderSSR\|from.*@analogjs/router' 2>/dev/null | head -10Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Search for actual calls to render() function (not imports)
rg -nP 'render\s*\(' packages/router/server/src --type ts -A 2 -B 2Repository: analogjs/analog
Length of output: 664
🏁 Script executed:
# Look at the full render.ts file to understand the complete flow
cat -n packages/router/server/src/render.ts | head -100Repository: analogjs/analog
Length of output: 2977
🏁 Script executed:
# Search for where the render factory result is called/exported
rg -nP 'export.*render|return.*render' packages/router/server/src --type tsRepository: analogjs/analog
Length of output: 471
🏁 Script executed:
# Look for test files that might test concurrent rendering
fd -t f 'spec\|test' packages/router/server --type ts | xargs catRepository: analogjs/analog
Length of output: 228
🏁 Script executed:
# Search for where the render function returned value is actually invoked
rg -nP 'render\(.*,.*,.*\)' packages/ apps/ -t ts -t js --type-add 'ts:*.ts' --type-add 'js:*.js' -A 2 | head -40Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Look at generated template files to understand how render() is used
fd -t f 'handler\|ssr' apps/ | grep -v node_modules | grep -v distRepository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Search for defineEventHandler patterns which is Nitro's way of defining SSR
rg -nP 'defineEventHandler|export default' packages/ apps/ --type ts --type js -A 3 | grep -B 3 -A 3 render | head -50Repository: analogjs/analog
Length of output: 2845
🏁 Script executed:
# Look at the generated index.ts exports to understand the package surface
cat packages/router/server/src/index.tsRepository: analogjs/analog
Length of output: 321
🏁 Script executed:
# Look at the actual Nitro handler that invokes render()
cat -n packages/vite-plugin-nitro/src/lib/utils/renderers.tsRepository: analogjs/analog
Length of output: 1997
🏁 Script executed:
# Search for how main.server.ts exports are used in the Nitro handler
rg -nP 'main\.server|__RENDER_FUNCTION__' packages/vite-plugin-nitro/src -A 5 -B 2Repository: analogjs/analog
Length of output: 4452
🏁 Script executed:
# Check if there are any comments or documentation about concurrency
rg -nP 'concurr|serial|parallel|mutex|lock|atomic' packages/router/server/src -A 2 -B 2Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Check if there are any concurrent/parallel rendering tests
fd -t f 'spec\|test' packages/router --exec cat {} \;Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Look at the h3/Nitro documentation or check if eventHandler enforces serialization
rg -nP 'eventHandler|h3' packages/vite-plugin-nitro/src/lib/utils/renderers.ts -B 3 -A 8Repository: analogjs/analog
Length of output: 1261
🏁 Script executed:
# Search for any existing issues or comments about this in the codebase
rg -nP 'race|concurrent|parallel|serial' packages/router -iRepository: analogjs/analog
Length of output: 671
🏁 Script executed:
# Check Nitro's event handling model - see if requests are serialized or concurrent
rg -nP 'from.*h3|from.*nitro' packages/ --type ts -A 2 -B 2 | head -50Repository: analogjs/analog
Length of output: 4120
🏁 Script executed:
# Check git history for render.ts to see recent changes
git log --oneline packages/router/server/src/render.ts | head -20Repository: analogjs/analog
Length of output: 157
🏁 Script executed:
# Check if there are any TODOs or FIXMEs related to concurrency/serialization
rg -nP 'TODO|FIXME|XXX|HACK' packages/router/server/src/render.ts -B 2 -A 2Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Search for any mutex/async lock implementations in the codebase
rg -nP 'Mutex|Lock|serialize|queue|drain' packages/ --type ts --type js -i | head -30Repository: analogjs/analog
Length of output: 4003
🏁 Script executed:
# Get detailed info about the commit that introduced resetComponentDefTViews
git show 320acf94 --statRepository: analogjs/analog
Length of output: 50371
🏁 Script executed:
# See the full diff of that commit
git show 320acf94 | head -150Repository: analogjs/analog
Length of output: 9174
🏁 Script executed:
# Check if there are any related PRs or issues mentioned
git log 320acf94^..320acf94 --format=fullerRepository: analogjs/analog
Length of output: 260
🏁 Script executed:
# Search for any concurrency-related discussions in PR metadata or comments
rg -nP 'concurrent|parallel|race|lock|serial|mutex' . --type md --type txt -i | grep -v node_modules | head -20Repository: analogjs/analog
Length of output: 3877
🏁 Script executed:
# Look at the Nitro handler more closely to understand the complete flow
rg -nP 'eventHandler|renderer|import' packages/vite-plugin-nitro/src/lib/utils/renderers.ts | head -30Repository: analogjs/analog
Length of output: 580
🏁 Script executed:
# Check if there's any documentation or TODO about concurrency in i18n code
rg -nP 'i18n|locale|render' packages/router/server/src --type ts -B 2 -A 2 | grep -i -E 'concurrent|todo|fixme|race|serial|lock' -B 2 -A 2Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Look at the i18nDefRegistryPlugin to understand how it works
fd -t f 'i18n.*plugin' packages/ -E node_modules | head -5Repository: analogjs/analog
Length of output: 161
🏁 Script executed:
# Check i18n provider code to understand the full locale switching flow
rg -nP 'provideI18n|loadTranslations' packages/router -A 5 --type ts | head -50Repository: analogjs/analog
Length of output: 3826
🏁 Script executed:
# Look for i18n tests that might test concurrent/parallel rendering
fd -t f 'i18n.*spec' packages/router/src --exec cat {} \; 2>/dev/null | head -100Repository: analogjs/analog
Length of output: 3259
🏁 Script executed:
# Check if there are any load tests or concurrent request tests
find packages/router apps -name '*.spec.ts' -o -name '*concurrent*' -o -name '*parallel*' | head -20Repository: analogjs/analog
Length of output: 633
🏁 Script executed:
# Look at actual usage - does anyone call render() concurrently?
rg -nP 'await.*render\(' packages/ apps/ --type ts -B 3 -A 3 | head -60Repository: analogjs/analog
Length of output: 5323
🏁 Script executed:
# Check Nitro's eventHandler documentation or behavior
rg -nP 'h3|eventHandler' packages/vite-plugin-nitro/src/lib/ -A 5 --type ts | grep -v node_modules | head -50Repository: analogjs/analog
Length of output: 4290
🏁 Script executed:
# Look at how h3/Nitro handles concurrent requests - check if there's built-in serialization
rg -nP 'concurrent|serial|queue|drain' packages/vite-plugin-nitro -i --type tsRepository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Check if the render function is called in sequence or concurrently in tests
rg -nP 'renderer\(|render\(' packages/vite-plugin-nitro -B 5 -A 5 --type ts | head -80Repository: analogjs/analog
Length of output: 1795
🏁 Script executed:
# Look at the actual test patterns for the i18n feature
cat packages/router/src/lib/i18n/provide-i18n.spec.ts | grep -A 20 'concurrent\|parallel' || echo "No concurrent tests found"Repository: analogjs/analog
Length of output: 84
🏁 Script executed:
# Check if there are any integration tests that exercise the SSR renderer
fd -t f 'ssr\|server.*spec' packages/router/server/srcRepository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Check if the code is documented as being for prerender-only
rg -nP 'prerender|single.*thread' packages/router/server/src -iRepository: analogjs/analog
Length of output: 41
🏁 Script executed:
# See the full render() function and its context
cat -n packages/router/server/src/render.ts | sed -n '37,84p'Repository: analogjs/analog
Length of output: 1533
🏁 Script executed:
# Check nitro prerender behavior to see if it's truly single-threaded
rg -nP 'prerender|concurrency|parallel' packages/vite-plugin-nitro/src/lib/build-server.ts -i -B 2 -A 2Repository: analogjs/analog
Length of output: 894
🏁 Script executed:
# Look for any comments or documentation about the expected usage
rg -nP 'TODO|FIXME|NOTE|@param|@returns' packages/router/server/src/render.ts -B 2 -A 2Repository: analogjs/analog
Length of output: 258
🏁 Script executed:
# Check the actual Nitro prerender documentation or behavior in this codebase
rg -nP 'prerender.*function|export.*prerender' packages/vite-plugin-nitro/src --type ts -A 10 | head -50Repository: analogjs/analog
Length of output: 1118
🏁 Script executed:
# Determine if Nitro's prerender is single-threaded or concurrent
rg -nP 'concurrency|workers|threads|parallel' packages/vite-plugin-nitro -i --type tsRepository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Check if there are runtime SSR tests (not just prerender)
fd -t f 'e2e\|integration' apps -name '*.spec.ts' | xargs grep -l 'render\|ssr' 2>/dev/null | head -5Repository: analogjs/analog
Length of output: 289
🏁 Script executed:
# Look at the commit message more carefully and check related comments
git show 320acf94 | grep -i 'concurr\|serial\|parallel\|race\|thread' -B 2 -A 2Repository: analogjs/analog
Length of output: 50371
🏁 Script executed:
# Check if there's documentation about when/how render() should be used
rg -nP 'render|SSR|server' packages/router/server/src --type ts --type md -B 1 -A 1 | grep -E '(render|SSR|concurrent|parallel)' | head -30Repository: analogjs/analog
Length of output: 2265
🏁 Script executed:
# Look for any usage patterns in the codebase
rg -nP 'from.*@analogjs/router/server|import.*render' packages/ apps/ --type ts -B 1 -A 3 | head -50Repository: analogjs/analog
Length of output: 3921
🏁 Script executed:
# Check if Nitro has any built-in request queuing/serialization
rg -nP 'serialize|queue|mutex|lock' packages/vite-plugin-nitro/src/lib/vite-plugin-nitro.ts -iRepository: analogjs/analog
Length of output: 41
Add a comment documenting the concurrency assumption or implement explicit serialization.
The process-global __ngComponentDefs Set is mutated in resetComponentDefTViews() (line 37–43) with no guards against concurrent requests. Under parallel SSR in production, two requests can interleave: Request A resets and awaits renderApplication(), Request B nulls the same def.tView mid-flight, then A resumes with stale or nulled component state.
Confirm one of the following before release:
- Nitro's prerender is single-threaded and runtime requests are serialized — if so, add a code comment at line 72 documenting this invariant (e.g., "Safe: Nitro SSR handler serializes per-request...").
- Otherwise, serialize the reset + render with an async mutex (e.g.,
pLimit) to prevent step 2 from interleaving with step 1's async work.
The PR title advertises correct i18n locale switching, so this must work reliably under concurrent load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router/server/src/render.ts` around lines 37 - 43, The global Set
__ngComponentDefs is mutated by resetComponentDefTViews() with no concurrency
guard, which can race with concurrent calls to renderApplication(); either
document the serialization guarantee or serialize the operations: add a clear
comment near resetComponentDefTViews() stating the runtime/Nitro SSR handler
serializes requests (so concurrent mutation cannot occur), or else wrap the
sequence that calls resetComponentDefTViews() and renderApplication() with an
async mutex/limit (e.g., pLimit or a simple Promise mutex) to ensure
resetComponentDefTViews() completes and renderApplication() runs without
interleaving; refer to resetComponentDefTViews and renderApplication in the
change.
…18n locale switching
Fixes three issues that prevented `provideI18n()` from switching locales
correctly across SSR requests in a single Node process.
1. `provideI18n()` no longer shadows the platform-level LOCALE on the
server — the provider is registered at environment level only on the
client, so the server-context value set by `provideServerContext()`
flows through to `injectLocale()` correctly.
2. `loadTranslationsRuntime()` now delegates to `@angular/localize`'s
`loadTranslations()` via dynamic import so each translation string is
parsed into the `{text, messageParts, placeholderNames}` shape that
`$localize.translate` expects.
3. A new `i18nDefRegistryPlugin` Vite plugin in `@analogjs/platform`
patches `@angular/core`'s `getComponentId()` during SSR transforms to
mirror every compiled component definition to
`globalThis.__ngComponentDefs`, bypassing the `ngServerMode` guard
that normally prevents registration on the server. The server renderer
reads this set before each `renderApplication()` call and nulls each
def's cached `tView` so that `consts()` re-evaluates `$localize`
tagged templates with the freshly loaded translations.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
320acf9 to
0154b25
Compare
PR Checklist
Fixes three issues that prevented
provideI18n()from switching locales correctly across SSR requests in a single Node process. Supersedes #2295 with a simpler approach that patches@angular/coreonce instead of instrumenting every user component file.Closes #
Affected scope
Recommended merge strategy for maintainer [optional]
What is the new behavior?
1. Server-context
LOCALEis no longer shadowed.provideI18n()previously providedLOCALEunconditionally at environment level with a value computed at module-load time, which on the server was alwaysdefaultLocale. This shadowed the platform-levelLOCALEset byprovideServerContext(). The provider is now registered only on the client; on the server, injection falls through to the platform-level value.2.
loadTranslationsRuntimestores parsed translations. The previous implementation set raw strings on$localize.TRANSLATIONS, which$localize.translate()cannot consume — it expects the{ text, messageParts, placeholderNames }shape produced byparseTranslation(). The helper now delegates to@angular/localize'sloadTranslations()via dynamic import.3.
provideAppInitializerreplacesENVIRONMENT_INITIALIZER. The old initializer returned an unawaited Promise, so translations could still be in flight when components began rendering.provideAppInitializerblocks bootstrap until translation loading completes.4. Per-request tView reset via
@angular/coretransform. A newi18nDefRegistryPluginin@analogjs/platformpatches@angular/core'sgetComponentId()during SSR transforms to mirror every compiled component definition toglobalThis.__ngComponentDefs, bypassing thengServerModeguard that normally prevents registration on the server. The server renderer reads this set before eachrenderApplication()call and nulls each def's cachedtViewso thatconsts()re-evaluates$localizetagged templates with the freshly loaded translations.5.
i18n-component-registry-plugindeleted. The v1 approach used an AST-walking Vite plugin that instrumented every user component file withɵregisterI18nComponentDef()calls. The v2 approach replaces it with a single string transform on@angular/core— no user-file instrumentation, nomagic-stringdependency, no new public API exports.Public API surface:
provideI18n,I18nConfig,injectSwitchLocale,loadTranslationsRuntime. No new exports added.ɵregisterI18nComponentDefandɵresetI18nComponentDefCacheremoved — no longer needed.Both
ssrBuildPluginandi18nDefRegistryPluginuse Vite'stransform: { filter, handler }pattern.Test plan
vitest run --config packages/router/vite.config.ts— 101 passed/fr→/de→/en→/frrequests each return correct locale translationspnpm buildpnpm testDoes this PR introduce a breaking change?
The runtime i18n API is still in beta.
ɵregisterI18nComponentDefandɵresetI18nComponentDefCache(added in the v1 PR and never released) are removed — they were framework plumbing with no external consumers.Other information
The tView reset is necessary because Angular caches
consts()results ondef.tView— that factory is where$localizetagged templates are evaluated. Without the reset, the first rendered locale's strings are frozen for the process lifetime. See angular/angular#68258 for the upstream issue filed againstangular/angular.