feat(@angular/ssr): introduce new hybrid rendering API #28139
Conversation
angular-robot
bot
added
detected: feature
alan-agius4
added
P4
alan-agius4
added
the
action: review
There was a problem hiding this comment.
Thanks for putting this together @alan-agius4! Really excited to see so much progress here.
I left a bunch of comments, but most of them are questions for my own understanding / clarifying edge case behavior. The rest are all relatively minor improvements to consider, so don't worry about them too much. Happy to chat and talk through some of the more complicated questions if that's easier.
I definitely appreciate the in depth JSDoc comments and tests, it really helps build up understanding of the change. 😁
alan-agius4
force-pushed
the
response-request-ssr
branch
4 times, most recently
from
bfa27cc to
6aa7a2d
Compare
There was a problem hiding this comment.
Agreed we should avoid Node-specific APIs as much as possible for maximum compatibility. I mainly just want to make sure that if we end up in a situation where we do need a Node API (maybe as a fallback, optimization, or other reason) we have a place to do that without breaking non-Node consumers.
| get hooks(): Hooks { | ||
| return AngularAppEngine.hooks; | ||
| } |
There was a problem hiding this comment.
Quick question: since we store hooks as a static property on a class, do we invoke all of the hooks for all instances of a class?
There was a problem hiding this comment.
Yes, although there are no scenarios where multiple instances of AngularAppEngine should be created.
There was a problem hiding this comment.
If we do end up with unit tests here (whether now or later), that would be a scenario where multiple AngularAppEngine objects are likely to be created.
There was a problem hiding this comment.
Yes, although there are no scenarios where multiple instances of AngularAppEngine should be created.
Would it be ok to store hooks on per-instance basis (vs having them as a static field)?
There was a problem hiding this comment.
In my opinion, relying on instances would be brittle and prone to user errors. It depends on users exporting the instance with a predetermined name, whereas using a static field eliminates this need. Setting these hooks is crucial for the dev-server, as it requires access to them to add HMR and live-reload scripts.
| * | ||
| * @example | ||
| * ```js | ||
| * const url = new URL('https://example.com/base/en/page'); |
There was a problem hiding this comment.
Quick question: is this the format of representing locale info that we have today (and we rely on) as well or it's a new format?
There was a problem hiding this comment.
This is the existing format.
alan-agius4
force-pushed
the
response-request-ssr
branch
from
4a72050 to
29b1992
Compare
| get hooks(): Hooks { | ||
| return AngularAppEngine.hooks; | ||
| } |
There was a problem hiding this comment.
Yes, although there are no scenarios where multiple instances of AngularAppEngine should be created.
Would it be ok to store hooks on per-instance basis (vs having them as a static field)?
packages/angular/ssr/src/render.ts
Outdated
| * @param value - The value to be checked. | ||
| * @returns True if the value is an Angular module (i.e., it has the `ɵmod` property), false otherwise. | ||
| */ | ||
| function isNgModule( |
There was a problem hiding this comment.
We can potentially use the reflectComponentType public API here, which would return null for NgModules, see https://angular.dev/api/core/reflectComponentType?tab=usage-notes.
There was a problem hiding this comment.
I do not think that reflectComponentType will work in this case because we do not have a component. What we need is a way to distinguish between a function that returns a promise and an NgModule.
alan-agius4
force-pushed
the
response-request-ssr
branch
6 times, most recently
from
e323c37 to
5c21e85
Compare
alan-agius4
force-pushed
the
response-request-ssr
branch
3 times, most recently
from
eb6d2d9 to
048c2f5
Compare
This commit introduces the new hybrid rendering API for Angular's Server-Side Rendering (SSR). The API aims to enhance the flexibility of SSR as discussed in angular/angular#56785 - This API is currently not accessible. - Additional work is required in the Angular CLI to: - Wire up the manifest. - Integrate other necessary components.
alan-agius4
force-pushed
the
response-request-ssr
branch
from
048c2f5 to
d9cb3a1
Compare
alan-agius4
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This commit introduces the new hybrid rendering API for Angular's Server-Side Rendering (SSR). The API aims to enhance the flexibility of SSR as discussed in angular/angular#56785
Notes