Skip to content

Commit

Permalink
refactor(core): throw an error when hydration marker is missing from DOM
Browse files Browse the repository at this point in the history
non-destructive hydration expects the DOM tree to have the same structure in both places.
With this commit, the app will throw an error if comments are stripped out by the http server (eg by some CDNs).

fixes angular#51160
  • Loading branch information
JeanMeche committed Jul 25, 2023
1 parent d886887 commit e2349d3
Show file tree
Hide file tree
Showing 9 changed files with 61 additions and 23 deletions.
2 changes: 2 additions & 0 deletions goldens/public-api/core/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ export const enum RuntimeErrorCode {
// (undocumented)
MISSING_HYDRATION_ANNOTATIONS = -505,
// (undocumented)
MISSING_HYDRATION_MARKER_COMMENT = 507,
// (undocumented)
MISSING_INJECTION_CONTEXT = -203,
// (undocumented)
MISSING_INJECTION_TOKEN = 208,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export const enum RuntimeErrorCode {
INVALID_SKIP_HYDRATION_HOST = -504,
MISSING_HYDRATION_ANNOTATIONS = -505,
HYDRATION_STABLE_TIMEDOUT = -506,
MISSING_HYDRATION_MARKER_COMMENT = 507,

// Signal Errors
SIGNAL_WRITE_FROM_ILLEGAL_CONTEXT = 600,
Expand Down
15 changes: 14 additions & 1 deletion packages/core/src/hydration/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import {ENABLED_SSR_FEATURES, PLATFORM_ID} from '../application_tokens';
import {Console} from '../console';
import {ENVIRONMENT_INITIALIZER, EnvironmentProviders, Injector, makeEnvironmentProviders} from '../di';
import {inject} from '../di/injector_compatibility';
import {formatRuntimeError, RuntimeErrorCode} from '../errors';
import {formatRuntimeError, RuntimeError, RuntimeErrorCode} from '../errors';
import {enableLocateOrCreateContainerRefImpl} from '../linker/view_container_ref';
import {enableLocateOrCreateElementNodeImpl} from '../render3/instructions/element';
import {enableLocateOrCreateElementContainerNodeImpl} from '../render3/instructions/element_container';
import {enableApplyRootElementTransformImpl} from '../render3/instructions/shared';
import {enableLocateOrCreateContainerAnchorImpl} from '../render3/instructions/template';
import {enableLocateOrCreateTextNodeImpl} from '../render3/instructions/text';
import {getDocument} from '../render3/interfaces/document';
import {TransferState} from '../transfer_state';
import {NgZone} from '../zone';

Expand Down Expand Up @@ -130,6 +131,18 @@ export function withDomHydration(): EnvironmentProviders {
useFactory: () => {
let isEnabled = true;
if (isBrowser()) {
// On the client, the DOM must contain a hydration marker or hydration is broken
const doc = getDocument();
const hydrationMarker = [...doc.body.childNodes].find(
node => node instanceof Comment && node.textContent === 'nghm');
if (!hydrationMarker) {
throw new RuntimeError(
RuntimeErrorCode.MISSING_HYDRATION_MARKER_COMMENT,
(typeof ngDevMode !== 'undefined' && ngDevMode) &&
'Angular hydration was requested on the client, but there was no hydration marker comment found in the node DOM tree. ' +
'Make sure the DOM is provided without changes by the HTTP server.');
}

// On the client, verify that the server response contains
// hydration annotations. Otherwise, keep hydration disabled.
const transferState = inject(TransferState, {optional: true});
Expand Down
19 changes: 17 additions & 2 deletions packages/platform-server/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ApplicationRef, InjectionToken, PlatformRef, Provider, Renderer2, StaticProvider, Type, ɵannotateForHydration as annotateForHydration, ɵENABLED_SSR_FEATURES as ENABLED_SSR_FEATURES, ɵInitialRenderPendingTasks as InitialRenderPendingTasks, ɵIS_HYDRATION_DOM_REUSE_ENABLED as IS_HYDRATION_DOM_REUSE_ENABLED} from '@angular/core';
import {ApplicationRef, InjectionToken, PlatformRef, Provider, Renderer2, StaticProvider, Type, ɵannotateForHydration as annotateForHydration, ɵENABLED_SSR_FEATURES as ENABLED_SSR_FEATURES, ɵIS_HYDRATION_DOM_REUSE_ENABLED as IS_HYDRATION_DOM_REUSE_ENABLED} from '@angular/core';
import {first} from 'rxjs/operators';

import {PlatformState} from './platform_state';
Expand All @@ -31,6 +31,19 @@ function createServerPlatform(options: PlatformOptions): PlatformRef {
]);
}

/**
* Creates a comment to mark the document as generated by SSR.
* Some CDNs have mechanisms to remove all comment node from HTML.
* This behaviour breaks hydration, so we'll detect on the client side if this
* marker comment is still available or else throw an error
*/
function appendMarkerComment(doc: Document) {
// Adding a ng hydration marken comment
const comment = doc.createComment('nghm');
doc.body.firstChild ? doc.body.insertBefore(comment, doc.body.firstChild) :
doc.body.append(comment);
}

/**
* Adds the `ng-server-context` attribute to host elements of all bootstrapped components
* within a given application.
Expand All @@ -55,10 +68,12 @@ function appendServerContextInfo(applicationRef: ApplicationRef) {
async function _render(platformRef: PlatformRef, applicationRef: ApplicationRef): Promise<string> {
const environmentInjector = applicationRef.injector;

const platformState = platformRef.injector.get(PlatformState);
appendMarkerComment(platformState.getDocument());

// Block until application is stable.
await applicationRef.isStable.pipe((first((isStable: boolean) => isStable))).toPromise();

const platformState = platformRef.injector.get(PlatformState);
if (applicationRef.injector.get(IS_HYDRATION_DOM_REUSE_ENABLED, false)) {
annotateForHydration(applicationRef, platformState.getDocument());
}
Expand Down
13 changes: 10 additions & 3 deletions packages/platform-server/test/hydration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ function convertHtmlToDom(html: string, doc: Document): HTMLElement {
return container;
}

function stripHydrationMarker(input: string): string {
return input; //.replace(/<!--nghm-->/s, '');
}

function stripTransferDataScript(input: string): string {
return input.replace(/<script (.*?)<\/script>/s, '');
}
Expand All @@ -105,7 +109,8 @@ function whenStable(appRef: ApplicationRef): Promise<void> {
function verifyClientAndSSRContentsMatch(ssrContents: string, clientAppRootElement: HTMLElement) {
const clientContents =
stripTransferDataScript(stripUtilAttributes(clientAppRootElement.outerHTML, false));
ssrContents = stripTransferDataScript(stripUtilAttributes(ssrContents, false));
ssrContents =
stripHydrationMarker(stripTransferDataScript(stripUtilAttributes(ssrContents, false)));
expect(clientContents).toBe(ssrContents, 'Client and server contents mismatch');
}

Expand Down Expand Up @@ -3987,7 +3992,7 @@ describe('platform-server hydration integration', () => {
let hostElement: Element;
if (this.isServer) {
hostElement = this.doc.createElement('portal-app');
this.doc.body.insertBefore(hostElement, this.doc.body.firstChild);
this.doc.body.insertBefore(hostElement, this.doc.body);
} else {
hostElement = this.doc.querySelector('portal-app')!;
}
Expand Down Expand Up @@ -4015,6 +4020,7 @@ describe('platform-server hydration integration', () => {
}

const html = await ssr(SimpleComponent);
debugger;
const ssrContents = getAppContents(html);

expect(ssrContents).toContain('<app ngh');
Expand All @@ -4033,7 +4039,8 @@ describe('platform-server hydration integration', () => {
stripUtilAttributes(clientRootNode.outerHTML, false);
expect(clientContents)
.toBe(
stripUtilAttributes(stripTransferDataScript(ssrContents), false),
stripHydrationMarker(
stripUtilAttributes(stripTransferDataScript(ssrContents), false)),
'Client and server contents mismatch');
});

Expand Down
26 changes: 13 additions & 13 deletions packages/platform-server/test/integration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ describe('platform-server integration', () => {
describe('render', () => {
let doc: string;
let expectedOutput =
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!<h1>fine</h1></app></body></html>';
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!<h1>fine</h1></app></body></html>';

beforeEach(() => {
// PlatformConfig takes in a parsed document so that it can be cached across requests.
Expand Down Expand Up @@ -768,7 +768,7 @@ describe('platform-server integration', () => {

expect(output).toBe(
'<html><head><title>fakeTitle</title></head>' +
'<body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'Works!<h1>fine</h1></app>' +
'<!--test marker--></body></html>');
});
Expand All @@ -779,7 +779,7 @@ describe('platform-server integration', () => {
renderModule(SVGServerModule, options);
const output = await bootstrap;
expect(output).toBe(
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<svg><use xlink:href="#clear"></use></svg></app></body></html>');
});

Expand Down Expand Up @@ -845,7 +845,7 @@ describe('platform-server integration', () => {
renderApplication(MyTransferStateAppStandalone, options) :
renderModule(MyTransferStateModule, options);
const expectedOutput =
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other"><div>Works!</div></app>' +
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other"><div>Works!</div></app>' +
'<script id="ng-state" type="application/json">{"some-key":"some-value"}</script></body></html>';
const output = await bootstrap;
expect(output).toEqual(expectedOutput);
Expand Down Expand Up @@ -954,7 +954,7 @@ describe('platform-server integration', () => {
renderModule(FalseAttributesModule, options);
const output = await bootstrap;
expect(output).toBe(
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<my-child ng-reflect-attr="false">Works!</my-child></app></body></html>');
});

Expand All @@ -964,7 +964,7 @@ describe('platform-server integration', () => {
renderModule(NameModule, options);
const output = await bootstrap;
expect(output).toBe(
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<input name=""></app></body></html>');
});

Expand All @@ -978,7 +978,7 @@ describe('platform-server integration', () => {
renderModule(HTMLTypesModule, options);
const output = await bootstrap;
expect(output).toBe(
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<div><b>foo</b> bar</div></app></body></html>');
});

Expand All @@ -988,7 +988,7 @@ describe('platform-server integration', () => {
renderModule(HiddenModule, options);
const output = await bootstrap;
expect(output).toBe(
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">' +
'<input hidden=""><input></app></body></html>');
});

Expand All @@ -1001,7 +1001,7 @@ describe('platform-server integration', () => {
const output = await bootstrap;
// title should be added by the render hook.
expect(output).toBe(
'<html><head><title>RenderHook</title></head><body>' +
'<html><head><title>RenderHook</title></head><body><!--nghm-->' +
'<app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app></body></html>');
});

Expand All @@ -1016,7 +1016,7 @@ describe('platform-server integration', () => {
// title should be added by the render hook.
expect(output).toBe(
'<html><head><title>RenderHook</title><meta name="description"></head>' +
'<body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app></body></html>');
'<body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app></body></html>');
expect(consoleSpy).toHaveBeenCalled();
});

Expand All @@ -1029,7 +1029,7 @@ describe('platform-server integration', () => {
const output = await bootstrap;
// title should be added by the render hook.
expect(output).toBe(
'<html><head><title>AsyncRenderHook</title></head><body>' +
'<html><head><title>AsyncRenderHook</title></head><body><!--nghm-->' +
'<app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app></body></html>');
});

Expand All @@ -1045,7 +1045,7 @@ describe('platform-server integration', () => {
// title should be added by the render hook.
expect(output).toBe(
'<html><head><meta name="description"><title>AsyncRenderHook</title></head>' +
'<body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app></body></html>');
'<body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app></body></html>');
expect(consoleSpy).toHaveBeenCalled();
});

Expand All @@ -1058,7 +1058,7 @@ describe('platform-server integration', () => {
renderModule(PendingTasksAppModule, options);
const output = await bootstrap;
expect(output).toBe(
'<html><head></head><body>' +
'<html><head></head><body><!--nghm-->' +
'<app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Completed: Yes</app>' +
'</body></html>');
});
Expand Down
4 changes: 2 additions & 2 deletions packages/platform-server/test/transfer_state_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {renderModule, ServerModule} from '@angular/platform-server';

describe('transfer_state', () => {
const defaultExpectedOutput =
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app><script id="ng-state" type="application/json">{"test":10}</script></body></html>';
'<html><head></head><body><!--nghm--><app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</app><script id="ng-state" type="application/json">{"test":10}</script></body></html>';

it('adds transfer script tag when using renderModule', async () => {
const STATE_KEY = makeStateKey<number>('test');
Expand Down Expand Up @@ -56,7 +56,7 @@ describe('transfer_state', () => {
const output =
await renderModule(EscapedTransferStoreModule, {document: '<esc-app></esc-app>'});
expect(output).toBe(
'<html><head></head><body><esc-app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</esc-app>' +
'<html><head></head><body><!--nghm--><esc-app ng-version="0.0.0-PLACEHOLDER" ng-server-context="other">Works!</esc-app>' +
'<script id="ng-state" type="application/json">' +
`{"testString":"\\u003C/script>\\u003Cscript>alert('Hello&' + \\"World\\");"}` +
'</script></body></html>');
Expand Down
2 changes: 1 addition & 1 deletion packages/tsconfig-build.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"target": "es2022",
// Keep the below in sync with ng_module.bzl
"useDefineForClassFields": false,
"lib": ["es2020", "dom"],
"lib": ["es2020", "dom", "dom.iterable"],
"skipLibCheck": true,
// don't auto-discover @types/node, it results in a ///<reference in the .d.ts output
"types": [],
Expand Down
2 changes: 1 addition & 1 deletion packages/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
},
"rootDir": ".",
"inlineSourceMap": true,
"lib": ["es2020", "dom"],
"lib": ["es2020", "dom", "DOM.Iterable"],
"skipDefaultLibCheck": true,
"skipLibCheck": true,
"types": ["angular"]
Expand Down

0 comments on commit e2349d3

Please sign in to comment.