refactor(core): throw an error when hydration marker is missing from DOM #51170

JeanMeche · 2023-07-25T22:19:59Z

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 #51160

I'll create a dedicate doc for the error in a subsequent PR.

PR Type

What kind of change does this PR introduce?

Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

No

packages/core/src/hydration/api.ts

packages/platform-server/test/integration_spec.ts

packages/platform-server/src/utils.ts

josephperrott

LGTM

Reviewed-for: dev-infra

AndrewKushnir

@JeanMeche thanks for the changes, everything looks good! The code is in the right location, just left a few comments with proposed improvements. Thank you.

packages/core/src/hydration/api.ts

packages/platform-server/src/utils.ts

packages/platform-server/test/hydration_spec.ts

packages/platform-server/test/integration_spec.ts

packages/core/src/hydration/api.ts

AndrewKushnir

@JeanMeche thanks for addressing the feedback! 👍 I've left a final comment and we should be ready for the next steps: presubmit and merge.

packages/platform-browser/test/hydration_spec.ts

josephperrott

LGTM

Reviewed-for: fw-security

AndrewKushnir

@JeanMeche thanks for addressing comments 👍 I did a final pass and added a couple more (all minor comments).

packages/core/src/errors.ts

packages/core/src/hydration/api.ts

AndrewKushnir

@JeanMeche thanks for the update 👍

AndrewKushnir

Reviewed-for: public-api

AndrewKushnir · 2023-08-01T20:16:20Z

@JeanMeche it looks like there is a merge conflict with the main branch. Could you please rebase and resolve conflicts when you get a chance? Thank you.

JeanMeche · 2023-08-01T21:13:08Z

@AndrewKushnir One final group of reviewers and we're good 👍

AndrewKushnir · 2023-08-01T21:37:59Z

Presubmit.

jessicajaniuk

reviewed-for: public-api

ngbot · 2023-08-01T23:33:49Z

I see that you just added the action: merge label, but the following checks are still failing:
     status "google-internal-tests" is failing
     status "pullapprove" is pending
     3 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir · 2023-08-01T23:38:21Z

Caretaker note: the presubmit is "green", the only outstanding thing is an extra review from the fw-security group (in addition to Joey's approval). The reason fw-security was added is this change, which adds dom.iterable to compilerOptions.lib list. I think Joey's approval in this case is enough and we can proceed with the merge.

dylhunn · 2023-08-04T15:31:17Z

@JeanMeche This PR has a merge conflict with the patch branch. I'm merging it into just minor. Feel free to create a patch rebase if you want to land it in patch.

dylhunn · 2023-08-04T15:31:51Z

This PR was merged into the repository by commit 0a38dc3.

AndrewKushnir · 2023-08-04T16:57:46Z

@JeanMeche it looks like this PR only made it into the main branch, which is targeting the next major. It'd be great to include this change into the upcoming v16.2.0. Could you please create a version of this PR targeting 16.2.x branch (and label is as "target: rc")? Thank you.

JeanMeche · 2023-08-04T19:57:43Z

@AndrewKushnir @dylhunn I made it available at #51276

…DOM (angular#51170) 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).

AndrewKushnir · 2023-08-04T21:16:02Z

@JeanMeche thanks a lot for creating a patch version of this PR! 👍

…DOM (#51170) (#51276) 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). PR Close #51276

AndrewKushnir · 2023-08-12T05:11:04Z

@JeanMeche please let me know if you want me to put together an error guide page for this error or if you get a chance to help (I think we have a lot of useful text in code comments, we can use them in the error guide). Without an error guide page, we'd only show an error code in prod mode (which is where the problem would happen). Thank you.

JeanMeche · 2023-08-12T08:13:25Z

@AndrewKushnir I've opened PR #51340 👍

angular-automatic-lock-bot · 2023-09-12T00:08:08Z

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.}

…DOM (angular#51170) 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 PR Close angular#51170

JeanMeche force-pushed the chore/ssr-marker branch 3 times, most recently from 49748d2 to 04f37f0 Compare July 26, 2023 00:18

AndrewKushnir reviewed Jul 26, 2023

View reviewed changes

JeanMeche force-pushed the chore/ssr-marker branch 2 times, most recently from 604aa26 to 28367ca Compare July 26, 2023 12:04

JeanMeche marked this pull request as ready for review July 26, 2023 14:14

pullapprove bot requested review from alxhub, jelbourn, josephperrott and pkozlowski-opensource July 26, 2023 14:14

josephperrott approved these changes Jul 26, 2023

View reviewed changes

pullapprove bot requested a review from josephperrott July 26, 2023 14:15

jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Jul 26, 2023

ngbot bot added this to the Backlog milestone Jul 26, 2023

AndrewKushnir reviewed Jul 28, 2023

View reviewed changes

JeanMeche force-pushed the chore/ssr-marker branch 2 times, most recently from 5d58766 to ce1283a Compare July 28, 2023 12:43

AndrewKushnir reviewed Jul 28, 2023

View reviewed changes

packages/platform-browser/test/hydration_spec.ts Outdated Show resolved Hide resolved

JeanMeche force-pushed the chore/ssr-marker branch 2 times, most recently from 4c9389c to df3f675 Compare July 31, 2023 10:02

josephperrott approved these changes Jul 31, 2023

View reviewed changes

AndrewKushnir reviewed Jul 31, 2023

View reviewed changes

packages/core/src/errors.ts Outdated Show resolved Hide resolved

packages/core/src/hydration/api.ts Outdated Show resolved Hide resolved

packages/core/src/hydration/api.ts Outdated Show resolved Hide resolved

JeanMeche force-pushed the chore/ssr-marker branch from df3f675 to ff1c380 Compare July 31, 2023 17:57

AndrewKushnir approved these changes Jul 31, 2023

View reviewed changes

AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 1, 2023

JeanMeche force-pushed the chore/ssr-marker branch from 01d9c4d to 824e3c2 Compare August 1, 2023 20:57

AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 1, 2023

jessicajaniuk approved these changes Aug 1, 2023

View reviewed changes

AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Aug 1, 2023

dylhunn added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Aug 4, 2023

dylhunn closed this in 0a38dc3 Aug 4, 2023

JeanMeche mentioned this pull request Aug 4, 2023

[16.2.x] refactor(core): throw an error when hydration marker is missing from DOM #51276

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2023

refactor(core): throw an error when hydration marker is missing from DOM #51170

refactor(core): throw an error when hydration marker is missing from DOM #51170

Uh oh!

Conversation

JeanMeche commented Jul 25, 2023

PR Type

Does this PR introduce a breaking change?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Aug 1, 2023

Uh oh!

JeanMeche commented Aug 1, 2023

Uh oh!

AndrewKushnir commented Aug 1, 2023

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Aug 1, 2023

Uh oh!

AndrewKushnir commented Aug 1, 2023

Uh oh!

dylhunn commented Aug 4, 2023

Uh oh!

dylhunn commented Aug 4, 2023

Uh oh!

AndrewKushnir commented Aug 4, 2023

Uh oh!

JeanMeche commented Aug 4, 2023

Uh oh!

AndrewKushnir commented Aug 4, 2023

Uh oh!

AndrewKushnir commented Aug 12, 2023

Uh oh!

JeanMeche commented Aug 12, 2023

Uh oh!

angular-automatic-lock-bot bot commented Sep 12, 2023

Uh oh!

AndrewKushnir left a comment •

edited

Loading