feat(platform-server): add analytics token for rendering

[CaerusKaru]

In order to analyze how a page was rendered, we add a new token
that can be populated and inserted into a rendered webpage. By
default, this token is just "platform-server", but can be
customized by the user depending on their given setup.

We do this here instead of in the individual rendering engines in
order to avoid analytics gaps for customers that may not use one
of the popular solutions, and instead opt to roll their own.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[AndrewKushnir]

nit: we can put the default value into the token factory (vs having it at the injection location):

Suggested change

	export const SERVER_CONTEXT = new InjectionToken<string>('Server.SERVER_CONTEXT');
	export const SERVER_CONTEXT = new InjectionToken<string>('SERVER_CONTEXT', {
	providedIn: 'root',
	factory: () => 'Angular platform-server'
	});

[CaerusKaru]

I considered this at first, but it didn't seem like it was worth the extra KBs tbh. Happy to move it there if you think it's better practice, but it seemed like a lateral move at best.

[AndrewKushnir]

Since this code remains on the server, extra bytes should be ok. My idea was to co-locate the definition and the default value, so that it's all in one place and is easier to manage later. Another approach might be to have an additional constant like const SERVER_CONTEXT_DEFAULT = 'Angular platform-server';, so we can have it next to the SERVER_CONTEXT one and use in the code where injection happens, but the factory approach looks more organic.

[AndrewKushnir]

I think we'd need to escape the context here to make sure values like --> do not break the markup once the document is serialized and parsed later, WDYT?

[CaerusKaru]

--> in a comment does not break the syntax. The spec is robust to cases like that, so I'm comfortable leaving out serialization for now.

[AndrewKushnir]

I did a very naive test to illustrate the scenario I had in mind. This doesn't exactly represent the serialization/deserialization that we do in the platform-server, but I was wondering if this might happen with our serialization too:

const div = document.createElement('div');
const comment = document.createComment('-->');
div.appendChild(comment);
const outputAsString = div.outerHTML;

const container = document.createElement('p'); // just used as a container
container.innerHTML = outputAsString;
console.log(container.firstChild.childNodes.length);
// Output: 2, the contents: NodeList(2) [comment, text ('-->')]
// Expecting: 1 comment node

[alan-agius4]

Couple of drive by comments

• I think that the value of the token to be from an enum. We want to use this for stats and thus we should have predefined values. I am thinking something along (ssr/ssg/appShell/other).
• I would be more keen in using something that is more queryable.

<meta property="ng:page-mode" content="SSR/SSG/AppShell/other" />

or

<aio-shell ng-version="13.2.2" ng-page-mode="SSR/SSG/AppShell/other">

• DI Tokens are problematic to be set during rendering/build time outside of the server bundle. This is because we cannot access the token with the same reference as the one bundled unless this is exported from the main.server.ts, similar to how we export renderModule. This would require migrations.

If we are going to move forward with this, I think it should be a parameter to the renderModule/renderApplication methods similar to how the index contents are provided.

[AndrewKushnir]

I've looked at this PR and at angular/universal#2666 once again and I think we may need both eventually :)

• page-level information on how this page was generated (added via a <meta> tag as Alan proposed)
• component/application-level information on whether this particular component was server-side-rendered or not

Given the fact that the renderModule and renderApplication functions operate on an application level (not the entire page level) and technically multiple applications can be bootstrapped on a page (potentially even a mix of SSR and CSR ones?), it feels like these functions should not append any page-level info into the output. Adding a <meta> tag to the page as a side-effect of running one of those functions also doesn't look right to me (for the same reason: there might potentially be a mix of different apps on a page). Instead, for the renderModule and renderApplication functions it makes sense to include an extra info on whether this particular app on a page was server-side-rendered or not (a boolean flag). However, this is something we should discuss further: whether such boolean flag would add value and in which scenarios it can be used (aside from stats).

The NgUniversal operates on a page-level basis and controls the entire page (starting from the index.html), it makes sense to me to expect that the NgUniversal would add some extra meta info to the output HTML to indicate which method was used (SSR/SSG/AppShell).

I'd vote for:

• updating the NgUniversal logic to add a <meta> tag as Alan proposed above, so that it's more query-able than a comment node
• revisiting the need of adding a flag to the renderModule and renderApplication functions (and propagate it to the rendered output as a host element attribute) as a part of the hydration efforts.

There are some downsides though, such as an extra complexity to query for this information on a page (we'd need to search for <meta> tag as well as all elements with a special attribute) and some extra bytes to ship to the browser.

@alan-agius4 @CaerusKaru I'm new to this part of the codebase, so I'd be curious to get your perspective on this.

// cc @jessicajaniuk

[alan-agius4]

The names renderApplication/renderModule might be slightly a confusing terminology. Because when using SSR/SSG/App-Shell, while you indeed bootstrap an application. What is actually needed is to render a single route, after the route is rendered the application is destroyed.

I do like what you are proposing above and I am fine either way. Although adding data in the host element does seem more future proof (Since there can be multiple apps on the same page.) . One drawback of using the meta element instead of the comment is that we'd need to parse the HTML document in Universal, which is currently not done.

We'd definitely wouldn't want to parse the document multiple times and therefore one thing that comes to mind is to change the document option in renderApplication to accept an actual Document.

export function renderApplication<T>(rootComponent: Type<T>, options: {
    appId: string;
+    document?: string;
-    document?: string | Document;
    url?: string;
    providers?: Array<Provider | ImportedNgModuleProviders>;
    platformProviders?: Provider[];
}): Promise<string>;

technically multiple applications can be bootstrapped on a page (potentially even a mix of SSR and CSR ones?),

Technically that is possible, although I highly doubt this is works and is supported at the moment.

[AndrewKushnir]

Quick update: we've discussed this topic offline with @CaerusKaru and also had a chat with @alan-agius4 and @jessicajaniuk. My takeaway from the discussions is that we need both application-level annotation via an attribute (to capture cases when renderApplication and renderModule fns are used without NgUniversal) as well as page-level annotation via a <meta> tag (to keep track of the NgUniversal usage).

If that's accurate, we'd need to land 3 items:

1. A new PR that would update the renderApplication and renderModule functions to accept a document as an object (as Alan proposed)
   • this would allow us to move the document parsing (string -> object) to the NgUniversal side
   • NgUniversal would be able to append the <meta> tag (and potentially use the DOM in the future for other features)
   • NgUniversal will invoke the renderModule or renderApplication and pass the document reference as an object
   • the renderModule or renderApplication would not need to do any parsing (unless a string is provided, which we'd need to support for backwards-compatibility)
   • [offtopic] the inline CSS processor uses HTML string as an input (see here). I'm wondering if it could eventually benefit from having parsed DOM to avoid extra re-parsing.
2. Land this PR after extra updates to add an attribute to the application host node
3. Update NgUniversal to add a <meta> tag (leveraging fix(@nguniversal/common): add a marker for SSR pages universal#2666 or in a new PR).

I'm familiar with the renderApplication and renderModule functions and can create a PR for the first item and update this thread once it's done, so we can proceed with the next items.

@alan-agius4 @CaerusKaru @jessicajaniuk please let me know if I missed something or there are any concerns.

Thank you.

[AndrewKushnir]

FYI, the PR that adds support for passing a document reference into the renderModule and renderApplication functions: #47032.

[alan-agius4]

Closing as this is obsolete.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}