feat: add SSR support #99
Conversation
WalkthroughAdds Angular SSR support to the kitchensink example: config updates, new server bootstrap and Express server, SSR route modes, client hydration, and an OIDC provider refactor. Also gates Angular OIDC initialization to browser-only. Package and TypeScript configs updated for SSR and Node types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant N as Node/Express
participant A as AngularNodeAppEngine
participant R as Renderer
U->>N: HTTP request
alt Static asset
N-->>U: Serve from /browser
else App route
N->>A: angularApp.handle(req)
A->>R: Render (SSR) with route modes
R-->>A: HTML/Response
A-->>N: writeResponseToNodeResponse(res)
N-->>U: SSR HTML response
end
sequenceDiagram
autonumber
participant B as Browser
participant S as Server (Node)
participant NG as Angular App
participant O as OIDC Provider
Note over S,NG: Server bootstrap
S->>NG: bootstrapApplication(App, server config)
NG-->>S: Pre-renders per serverRoutes
Note over B,NG: Client hydration
B->>NG: bootstrapApplication(App, client config)
NG->>NG: provideClientHydration(withEventReplay())
NG->>O: OIDC init (browser-only)
O-->>NG: Ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
I don’t see the benefit of introducing a bespoke app/oidc.ts file.
If the goal is simply not to clutter app.config.ts, that’s fine, but app/oidc.ts is not a standard Angular location. If we move it, the natural location seems to be app/services/oidc.service.ts (minimizing distance between related concerns).
I also disagree with bypassing Angular DI here.
- The example should communicate that the
Oidc.providecallback runs in an injectable context. Usingfetchhides that fact. - The idiomatic Angular way is to use
HttpClient. That’s how you get interceptors, standardized error handling, and mocking/testability. - If every library starts bypassing DI, we end up with Angular’s complexity without any of its advantages.
Do I think Angular’s DI design has flaws? Yes. For example:
- The fact that interceptors are always global.
- The lack of async middleware patterns, or a Promise-based API.
- The Observable-only approach for deferred results.
But these are API design issues in Angular itself. They don’t justify us bypassing the framework’s own conventions.
I get the concern: if someone sets up a global auth interceptor, you could trigger a loop (config fetch → needs token → OIDC not initialized). But that’s ultimately on Angular’s design and the user’s interceptor setup, not on us. The example should show the correct, idiomatic approach, not a one-off workaround.
There was a problem hiding this comment.
1- it's not strange having a ts file with configuration to pass inside app.config. It surely could be relocated, but definitely not inside oidc.service.ts, cause it's not an injectable service, but it is its provider.
2- fetch is necessary to make your async parameters work outside the http interceptors pipe. if you analyze the code you can find that interceptors need the Oidc instance to be defined to work, but it can't be defined without its parameters. So any http request to get config parameters must be runned outside angular http, you can also see what keycloak-angular suggests: https://github.com/mauriciovigolo/keycloak-angular?tab=readme-ov-file#load-the-keycloak-configuration-using-an-external-file.
3- it's not a workaround, it is the only way it could work
There was a problem hiding this comment.
There was a problem hiding this comment.
1- it's not strange having a ts file with configuration to pass inside app.config. It surely could be relocated, but definitely not inside oidc.service.ts, cause it's not an injectable service, but it is its provider.
I get your point, it’s config, not a service, so not obvious to colocate under services/oidc.service.ts. But Angular doesn’t have a canonical place for “provider factories,” and colocating with the service is perfectly legitimate because that’s where users will look.
Especially since oidc.service.ts here is already opinionated: it’s not a reusable generic, it’s an extended instance bound to a concrete ID token shape and autoLogin flag.
The other adapters think of Oidc as a singleton. Oidc is not a singleton. There just happen to be only one instance in most app.
Extracting a provideOidc is fine stylistically, but in my view it belongs next to the service itself. If we can’t agree, the neutral default is to just keep it inline in app.config.ts and let users decide how they want to organize.
2- fetch is necessary to make your async parameters work outside the http interceptors pipe.
This is where I think there’s a misunderstanding of Angular’s lifecycle.
By the time the callback passed to Oidc.provide is invoked, the Oidc service has already been instantiated and is injectable. Until the callback resolves, synchronous properties like oidc.$decodedIdToken() won’t work, but async methods like oidc.getAccessToken() are safe if ever called they’ll await initialization.
There is a possibility of a deadlock (interceptor waiting on OIDC init while OIDC init triggers interceptor) but it only happens if every request is blindly piped through an interceptor that injects tokens unconditionally. That’s not the pattern we recommend. The idiomatic pattern is:
const TODO_API_URL = 'https://jsonplaceholder.typicode.com/todos';
export const todoApiInterceptor: HttpInterceptorFn = (req, next) => {
const oidc = inject(Oidc);
if (!req.url.startsWith(TODO_API_URL)) {
return next(req);
}
return from(oidc.getAccessToken()).pipe(
switchMap(({ isUserLoggedIn, accessToken }) => {
if (!isUserLoggedIn) {
throw new Error("Assertion Error: Call to the TODO API while the user isn't logged in.");
}
return next(req.clone({ setHeaders: { Authorization: `Bearer ${accessToken}` } }));
})
);
};This avoids the deadlock because we bail early for requests that aren’t part of the API. If someone misconfigures and intercepts everything (including OIDC’s own config fetch), that’s on Angular’s global-interceptor design, to have lest this footgun, not on us.
To prove it’s robust and not a coincidence: you can even put provideHttpClient() after Oidc.provide(). It still works because instantiation happens first, then provideAppInitializer callbacks run.
Bottom line: instantiation ≠ initialization. Initializers run after all providers are instantiated, so there’s no cycle.
you can also see what keycloak-angular suggests
Yes, and their approach is exactly my point. They couldn’t make it work inside Angular’s DI rules, so they punted to fetch. That’s on them. Our approach keeps things idiomatic, testable, and mockable within Angular’s DI system. That’s a win.
3- it's not a workaround, it is the only way it could work
It’s demonstrably not the only way. The current example app is deployed with HttpClient for config and works fine: https://example-angular.oidc-spa.dev/.
So my position is: stick with HttpClient. It’s idiomatic Angular, avoids bypassing DI, and keeps interceptors/testability intact. Resorting to fetch is unnecessary and only entrenches a pattern that makes Angular code less Angular.
There was a problem hiding this comment.
There is a possibility of a deadlock (interceptor waiting on OIDC init while OIDC init triggers interceptor) but it only happens if every request is blindly piped through an interceptor that injects tokens unconditionally. That’s not the pattern we recommend.
this isn't something an angular developer would do, interceptor are great because they are global and they can be used to add business logic on every http requests (also writing interceptors inside services is a bad practice), making an interceptor for every api is an anti pattern using Angular.
They couldn’t make it work inside Angular’s DI rules, so they punted to fetch
that's not what they did. As angular developers, they know that fetching configuration files is not supposed to be intercepted in any way during request, so they correctly use an alternative way that avoids inserting the request in the angular http pipeline. They would be able to use http client because they haven't strict checks on keycloak authenticate/ready. Instead Oidc interceptors and guards are strictly coupled to prInitialized.
Resorting to fetch is unnecessary and only entrenches a pattern that makes Angular code less Angular.
Avoid to publish an interceptor utility because you want to use http client inside the configuration is a downgrade of the UX.
It's really a bad idea to inject Oidc on something that should be already provided by the library, cause authentication is its main scope.
There was a problem hiding this comment.
ok the memory leak is here:
Line 297 in ef21fb1
Calling subscribe to this subject leave an unsubscribed subscription in the memory on every run of the interceptor, cause this subject never completes. In this scenario we need to explicitly unsubscribe when the decodedIdTokenAccess check is finished (should be after this line:
Line 335 in ef21fb1
// L297
const subscription = (instance.decodedIdTokenAccess = new Subject<void>()).subscribe(
() => (isDecodedIdTokenAccessed = true)
);
---
// L335
instance.decodedIdTokenAccess = undefined;
subscription.unsubscribe();There was a problem hiding this comment.
Let me know what you think!
Let's break it down to be sure I understand the complete logic (let me know if my assumptions are correct or not).
Tracking decodedIdToken Access
let isDecodedIdTokenAccessed = false;
const subscription = (instance.decodedIdTokenAccess = new Subject<void>()).subscribe(
() => (isDecodedIdTokenAccessed = true)
);- This is made to check if the shouldInjectAccessToken function tries to read the contents of the ID token. This will cause deadlocks.
The "Dry Run": Simulating States
try {
shouldInjectAccessToken = getShouldInjectAccessToken(req);
} catch (error) {
// It failed, probably because OIDC service isn't ready.
}
if (shouldInjectAccessToken === undefined) {
for (const isUserLoggedIn of [false, true]) {
// ... loops and overrides ...
try {
shouldInjectAccessTokenOrError = getShouldInjectAccessToken(req);
} catch (error) {
shouldInjectAccessTokenOrError = { error };
}
// ... store result ...
}
}- The interceptor first tries to call your getShouldInjectAccessToken function directly.
- If the OIDC service isn't initialized yet, accessing properties like oidc.isUserLoggedIn would throw an error. The try...catch handles this.
- If it fails (shouldInjectAccessToken remains undefined), the interceptor performs a "dry run". It loops through all possible authentication states (isUserLoggedIn: true/false, isNewBrowserSession: true/false).
- In each simulated state, it temporarily overrides the service's properties (e.g., instance.#isUserLoggedIn_override = true;) and calls your getShouldInjectAccessToken function again.
- It stores the result (or any error thrown) for each simulated state in the shouldInjectAccessTokenByState array. This creates a "decision map" it can use later once the real state is known.
Cleanup and Validation
instance.decodedIdTokenAccess = undefined;
subscription.unsubscribe();
if (
!instance.providerAwaitsInitialization &&
isDecodedIdTokenAccessed &&
!instance.allowDecodedIdTokenAccessInShouldInjectAccessToken
) {
throw new Error(...);
}- instance.decodedIdTokenAccess is set to undefined to deactivate the tracking mechanism for the rest of the request lifecycle, the subscription is also unsubscribed.
- It then checks the isDecodedIdTokenAccessed flag. If you accessed the ID token when you weren't supposed to, it throws a helpful, explicit error with a link to the documentation.
The Main Logic: To Inject or Not to Inject
if (
shouldInjectAccessToken === false ||
// ... check if all dry runs returned false ...
) {
return next(req); // No token needed, pass request along.
}
// ... Deadlock detection logic ...
return from(instance.getAccessToken()).pipe(
switchMap(({ accessToken }) => {
// ... more logic ...
return next(req.clone({ setHeaders: { Authorization: `Bearer ${accessToken}` } }));
})
);- Early Exit: If the decision is clearly false (either from the initial synchronous call or from the dry run), the interceptor does nothing and simply forwards the original request down the chain with return next(req).
- Deadlock Detection: It sets a 4-second timer. If getting the access token takes too long, it logs a warning to the console, alerting the developer to a probable deadlock situation (e.g., the app initialization depends on an API call that is waiting for the app to initialize).
- Get Token: If a token might be needed, it calls instance.getAccessToken(), which is an async operation.
- Once the token is retrieved, the logic inside switchMap executes.
- It re-evaluates shouldInjectAccessToken using the "decision map" if it wasn't determined synchronously.
- If the final decision is false, it forwards the original request.
- If true, it clones the request, adds the Authorization: Bearer ${accessToken} header, and forwards the new, cloned request down the chain.
There was a problem hiding this comment.
It is a very big and complex interceptor and it defines a strict behaviour on the bearer token addition.
Is it completely positive and safe for the user to be forced in those rails? maybe yes.
I have concerns regarding possible false positives on the timeout callback due to slow connection, and on the maintainability of this function during the future evolutions of oidc-spa.
Overall my opinion remains positive
There was a problem hiding this comment.
Thanks a lot for the detailed review!
I’m really looking forward to going through it. I just need to postpone for a few days to focus on something else first, but I’ll definitely come back to it soon.
There was a problem hiding this comment.
Some people are currently asking for TanStack Start support.
Right now, there’s no solid authentication solution for that platform apart from the Clerk SDK, so there’s a real opportunity to establish a standard before the space fills up.
Angular support matters just as much, it’s just less time-sensitive, which is why I’m postponing it for a few days, not because I don’t care.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/angular-kitchensink/src/app/oidc.ts (2)
14-23: Previous discussion unresolved: fetch vs HttpClient.This implementation uses
fetchto avoid Angular's HTTP interceptor chain. As discussed in previous review comments, this approach bypasses Angular's DI system and loses testability/mockability benefits.The concern about interceptor deadlock can be addressed with proper interceptor design (early bailout for non-API URLs). The current deployed example demonstrates that
HttpClientworks correctly with this pattern.Additionally, Line 15 has a typo: "runned" → "run".
1-24: Previous discussion unresolved: file organization.The file location and extraction of
provideOidcinto a separateoidc.tswas previously discussed. Angular lacks a canonical location for provider factories. Options include:
- Keeping inline in
app.config.ts- Colocating with
services/oidc.service.ts- Current approach (separate file)
This is primarily a style preference with no technical issues.
🧹 Nitpick comments (3)
examples/angular-kitchensink/src/server.ts (2)
52-61: Consider graceful error handling for server startup.The server startup error handling currently throws the error, which will crash the process. For production deployments, consider implementing graceful error handling with proper logging and potentially retry logic.
Apply this pattern for better error handling:
if (isMainModule(import.meta.url)) { const port = process.env['PORT'] || 4000; - app.listen(port, (error) => { - if (error) { - throw error; - } - - console.log(`Node Express server listening on http://localhost:${port}`); - }); + const server = app.listen(port, () => { + console.log(`Node Express server listening on http://localhost:${port}`); + }); + + server.on('error', (error) => { + console.error('Failed to start server:', error); + process.exit(1); + }); }
1-67: Consider adding production-ready middleware for security and performance.The current Express setup is minimal and suitable for development. For production deployments, consider adding:
Security middleware:
helmetfor security headers- CORS configuration if needed for cross-origin requests
- Rate limiting to prevent abuse
- Request size limits to prevent DoS
Performance middleware:
- Compression (gzip/brotli) for responses
- Request logging for observability
Example additions:
import helmet from 'helmet'; import compression from 'compression'; // Security headers app.use(helmet({ contentSecurityPolicy: { directives: { defaultSrc: ["'self'"], // Configure CSP for your OIDC provider } } })); // Compression app.use(compression()); // Request size limits app.use(express.json({ limit: '10mb' })); app.use(express.urlencoded({ extended: true, limit: '10mb' }));Note: This is a recommended enhancement for production readiness, not a blocking issue for the SSR feature itself.
examples/angular-kitchensink/package.json (1)
32-35: Align patch versions for Angular packages
- Update
@angular/platform-server,@angular/router, and other Angular dependencies from^20.3.0to^20.3.3to match@angular/ssr.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/angular-kitchensink/angular.json(1 hunks)examples/angular-kitchensink/package.json(2 hunks)examples/angular-kitchensink/src/app/app.config.server.ts(1 hunks)examples/angular-kitchensink/src/app/app.config.ts(2 hunks)examples/angular-kitchensink/src/app/app.routes.server.ts(1 hunks)examples/angular-kitchensink/src/app/oidc.ts(1 hunks)examples/angular-kitchensink/src/main.server.ts(1 hunks)examples/angular-kitchensink/src/server.ts(1 hunks)examples/angular-kitchensink/tsconfig.app.json(1 hunks)src/angular.ts(3 hunks)
🔇 Additional comments (15)
examples/angular-kitchensink/package.json (1)
35-35: Confirm Express v5 migration and compatibility.The project is using
express@^5.1.0, which is a major version upgrade from v4 with breaking changes. According to the Express v5 migration documentation, there are significant changes in route matching semantics, middleware behavior, and internal APIs.Ensure that:
- The migration from Express v4 to v5 is intentional and the SSR server code (examples/angular-kitchensink/src/server.ts) has been reviewed against the Express v5 migration guide.
- All middleware and routing patterns are compatible with v5 semantics.
- Error handling follows v5 conventions.
As per learnings, you should test middleware chains thoroughly and verify third-party middleware compatibility before deploying to production.
examples/angular-kitchensink/tsconfig.app.json (1)
7-7: LGTM! Node types correctly added for SSR support.Adding
"node"to the compiler types array is the correct approach for enabling Node.js type declarations in the TypeScript compilation. This is essential for SSR code that uses Node.js APIs (e.g.,path,process,import.meta.dirname) and aligns with the@types/nodedependency added in package.json.examples/angular-kitchensink/angular.json (1)
24-29: LGTM! SSR configuration correctly added.The SSR configuration follows Angular's standard conventions:
serverpoints to the server bootstrap entry (src/main.server.ts)outputMode: "server"enables server-side rendering modessr.entrypoints to the Express server setup (src/server.ts)This configuration correctly wires up the SSR infrastructure introduced in this PR.
examples/angular-kitchensink/src/app/app.routes.server.ts (1)
3-11: Verify prerender mode for root path with OIDC.The root path (
'') is configured withRenderMode.Prerender, while authenticated routes like'protected'and'admin-only'useRenderMode.Client.Ensure that:
- The root path does not require authentication (if it does, it should use
RenderMode.Clientinstead)- Prerendering the root path with OIDC initialization won't cause issues (the browser-gating added in src/angular.ts should prevent OIDC from running during SSR, which is correct)
If the root path requires authentication or displays user-specific content, consider using
RenderMode.Clientinstead ofRenderMode.Prerender.src/angular.ts (2)
214-217: LGTM! Browser-gating correctly prevents SSR OIDC initialization.The platform detection and early return pattern correctly prevents OIDC initialization during server-side rendering. This is essential because OIDC relies on browser-only APIs (localStorage, sessionStorage, window navigation, etc.) that are not available in the Node.js SSR environment.
The implementation:
- Uses Angular's standard
PLATFORM_IDandisPlatformBrowserfor platform detection- Returns early when not in browser context, preventing any OIDC-related code execution on the server
- Applies the same pattern consistently in both
provide()andprovideMock()methodsThis addresses the SSR compatibility requirements while maintaining the existing client-side behavior.
262-265: LGTM! Mock provider correctly gated for browser-only execution.The browser platform check is consistently applied to the mock provider, ensuring symmetric behavior between the real and mock OIDC implementations during SSR. This prevents mock OIDC initialization on the server, which is the correct approach as noted in the past review comments.
examples/angular-kitchensink/src/server.ts (1)
30-36: Verify aggressive cache duration for static assets.The
maxAge: '1y'cache duration is very aggressive. While this is appropriate for hashed/fingerprinted assets (which Angular typically generates), ensure that:
- All static assets in the browser dist folder have cache-busting mechanisms (content hashes in filenames)
- The Angular build is configured with
outputHashing: "all"(currently set for production in angular.json, which is correct)If assets are not consistently hashed, consider reducing the maxAge or implementing a more granular caching strategy.
examples/angular-kitchensink/src/main.server.ts (1)
1-7: LGTM! Server bootstrap correctly implemented.The server-side bootstrap follows Angular's standard SSR pattern:
- Imports and uses
BootstrapContextfor server-specific initialization- Delegates to
bootstrapApplicationwith the server config- Exports the bootstrap function as default for the Angular CLI to consume
This correctly wires the server-side application entry point with the SSR configuration.
examples/angular-kitchensink/src/app/app.config.ts (2)
7-7: LGTM! Client hydration correctly configured for SSR.The addition of
provideClientHydration(withEventReplay())is the correct setup for Angular SSR:
provideClientHydration()enables Angular's hydration feature, which reuses the server-rendered DOM instead of destroying and recreating it on the clientwithEventReplay()captures and replays user interactions that occur before hydration completes, providing a better user experience by not losing early user actionsThis complements the SSR setup and ensures optimal performance and user experience for the server-rendered application.
Also applies to: 21-21
12-12: LGTM! OIDC configuration refactored to dedicated module.Moving the OIDC provisioning logic to a dedicated
./oidcmodule improves code organization and maintainability. The API remains clean withprovideOidc(environment.useMockOidc), maintaining the same behavior while improving separation of concerns.Also applies to: 20-20
examples/angular-kitchensink/src/app/app.config.server.ts (3)
1-4: LGTM!The imports follow Angular SSR conventions and properly reference the client config and server routes.
6-8: LGTM!The server config follows Angular SSR best practices with
provideServerRenderingand route-based rendering configuration.
10-10: LGTM!The config export correctly merges client and server configurations using Angular's
mergeApplicationConfigutility.examples/angular-kitchensink/src/app/oidc.ts (2)
1-7: LGTM!The imports and type definition are clean and appropriate for the OIDC configuration setup.
9-13: LGTM!The mock OIDC provider branch is clean and uses the library's built-in mock functionality appropriately.
Summary by CodeRabbit