Rule based html validation against hydration #2493

yiwen101 · 2024-04-03T09:32:09Z

What is the purpose of this pull request?

Work on #2025

Overview of changes:
Add extensible rule based validation check against hydration

Anything you'd like to highlight/discuss:
As commented in #2025, it turns out that modern browsers are quite robust and can tolerate most of the common html violations.
But still ideal to add check for violations that are fatal, and make checks for other violations optional.

I made the changes with extensibility in mind, as it is very likely that we will add further rules in the future.
Have not added test/documentation as not sure whether current implementation is desirable. Review required.

Testing instructions:
Add following in index.md

<table class="table">
   <tr>
     <th>Task ID</th>
     <th>Task</th>
     <th>Estimated Effort</th>
     <th>Prerequisite Task</th>
   </tr>
   <tr>
     <td>E</td>
     <td>Planning for next version</td>
     <td>1 man day</td>
     <td>D</td>
   </tr>
 </table>

Proposed commit message: (wrap lines at 72 characters)
Rule based html validation against hydration

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

codecov · 2024-04-03T09:36:04Z

Codecov Report

Attention: Patch coverage is 56.81818% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 51.15%. Comparing base (104b46c) to head (addfded).

❗ Current head addfded differs from pull request most recent head ff5634c. Consider uploading reports for the commit ff5634c to get more accurate results

Files	Patch %	Lines
packages/core/src/utils/htmlValidationUtil.ts	57.14%	16 Missing and 2 partials ⚠️
packages/core/src/Page/index.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2493      +/-   ##
==========================================
+ Coverage   51.01%   51.15%   +0.14%     
==========================================
  Files         124      125       +1     
  Lines        5389     5399      +10     
  Branches     1162     1157       -5     
==========================================
+ Hits         2749     2762      +13     
+ Misses       2350     2345       -5     
- Partials      290      292       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yucheng11122017

Hi @yiwen101 thanks for this PR! It seems very interesting and will definitely be useful for our users.

One thing I wanted to ask is if the RuleBuilder is necessary? Could you initialise the Rule directly using a constructor?

Could you also add some test cases to test htmlValidationUtil.ts functionality? Like show that there is a warning/error when a table is missing a body and so on.

Another thing with regards to code quality is maybe we can abstract out the actual rules (like table needed a tbody) to a seperate file from htmlValidationUtil.ts. Then htmlValidationUtil.ts will load in these rules. So htmlValidationUtil.ts only handles creating rules while this other file will handle the actual rule content.

kaixin-hc · 2024-04-05T16:37:32Z

Thanks for the extensibility considerations! I can definitely see it the code and I agree with the overall approach.

I second @/yucheng11122017 's comments though on whether we can use a constructor. I also don't like the idea of the pushRuleAndReset, because I feel like it opens the possibility where someone forgets to push a rule and maybe one of the other setter functions and then you don't get a well formed rule. If the user is expected to call all of those functions, it makes sense to me that they are all called at once.

packages/core/src/utils/htmlValidationUtil.ts

jingting1412

Thanks for working on this! Just some nits on improving logger messages and property names. It would be good to add some tests and documentation for this as well.

Similar to other reviewers, I also think that encapsulating the existing rules to another class for example would be good to better manage rule registration and execution.

packages/core/src/utils/htmlValidationUtil.ts

yiwen101 · 2024-04-17T07:36:28Z

@kaixin-hc @tlylt @yucheng11122017 @jingting1412

Thank you for all the comments and sorry about the later reply.

On second thought, I feel that although this is a quite simple, trivial and straightforward addition, the current code still complicating it unnecessarily. I wish to remove all the over-engineered parts, and reduce this PR to a simple validation of table tag has table body (the only violation that for sure leads to hydration as of now). What is your guys idea on this?

Let me explain where the current design comes from, and why it sucks.

Initially I want to add an extensive rule based checkers. However as commented here, it turns out that modern browsers are quite robust and can tolerate most of the common html violations (eg: nesting block level in inline level, missing closing tag, missing quotation for href...) . So causeHydration tag is designed to differentiate harmless violations and violations that leads to hydration.

However, since harmless violations are so pervasive (eg: even in 404.md), the false positive rate is unacceptable. Logging check results for them may bury and hide the actual, valuable logs. I see no point wasting computation power and brain cell to fill the mental bandwidth of log reader with results that they do not need to know and do not need to fix.

With "table body missing" being the only known violations that will for sure leads to hydration, I want to remove all over-engineered parts and make it the simplest, most straightforward form. What are you thoughts on this?

kaixin-hc · 2024-04-17T08:36:05Z

@yiwen101 Thanks for sharing your reasoning! Yes, it was a good surprise to me as well how robust current browsers are.

As shared in earlier comments I have some issues with the code in this "over-engineered" form. I support a PR specifically targeting "table body missing" especially if it's a small change; we can always change the code that checks this if we discover that there are more cases we must check.

yucheng11122017 · 2024-04-17T14:42:53Z

@kaixin-hc @tlylt @yucheng11122017 @jingting1412

Thank you for all the comments and sorry about the later reply.

On second thought, I feel that although this is a quite simple, trivial and straightforward addition, the current code still complicating it unnecessarily. I wish to remove all the over-engineered parts, and reduce this PR to a simple validation of table tag has table body (the only violation that for sure leads to hydration as of now). What is your guys idea on this?

Let me explain where the current design comes from, and why it sucks.

Initially I want to add an extensive rule based checkers. However as commented here, it turns out that modern browsers are quite robust and can tolerate most of the common html violations (eg: nesting block level in inline level, missing closing tag, missing quotation for href...) . So causeHydration tag is designed to differentiate harmless violations and violations that leads to hydration.

However, since harmless violations are so pervasive (eg: even in 404.md), the false positive rate is unacceptable. Logging check results for them may bury and hide the actual, valuable logs. I see no point wasting computation power and brain cell to fill the mental bandwidth of log reader with results that they do not need to know and do not need to fix.

With "table body missing" being the only known violations that will for sure leads to hydration, I want to remove all over-engineered parts and make it the simplest, most straightforward form. What are you thoughts on this?

This is great reasoning and very well thought through and researched! I am ok to keep it simple :)

yiwen101 · 2024-04-17T17:12:07Z

@kaixin-hc @yucheng11122017
Thank you for your response and input. I have rewritten the code and added test and docs.

yucheng11122017

Some comments! Thanks for the work

docs/devGuide/design/serverSideRendering.md

packages/core/src/utils/htmlValidationUtil.ts

packages/core/test/unit/utils/HtmlValidationUtil.test.ts

packages/core/test/unit/utils/html_str/correct_html.txt

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

yucheng11122017

Some nits

packages/core/src/utils/htmlValidationUtil.ts

packages/core/test/unit/utils/HtmlValidationUtil.test.ts

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

EltonGohJH

IMO, good to go.
After resolving all comments.

packages/core/src/utils/htmlValidationUtil.ts

This reverts commit c052a4d.

EltonGohJH

LGTM!

yucheng11122017

LGTM!
@yiwen101 you can trying merging this PR yourself :)

yiwen101 · 2024-04-18T11:32:06Z

LGTM!
@yiwen101 you can trying merging this PR yourself :)

Noted, thanks.

github-actions · 2024-04-18T11:34:46Z

@yiwen101 Each PR must have a SEMVER impact label, please remember to label the PR properly.

* Add rule based html validation against Vue hydration issue * Improve docs * Update test * Add rule to check against table tag without tboby * Update docs/devGuide/design/serverSideRendering.md * Update packages/core/test/unit/utils/HtmlValidationUtil.test.ts * Update packages/core/src/utils/htmlValidationUtil.ts * Update packages/core/test/unit/utils/HtmlValidationUtil.test.ts --------- Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

yiwen101 and others added 2 commits April 3, 2024 17:25

Add rule based html validation

2b0003b

Merge branch 'master' into hydration_rule

addfded

yucheng11122017 reviewed Apr 5, 2024

View reviewed changes

yucheng11122017 force-pushed the master branch 2 times, most recently from cb84513 to 69ec838 Compare April 5, 2024 06:21

tlylt reviewed Apr 6, 2024

View reviewed changes

packages/core/src/utils/htmlValidationUtil.ts Outdated Show resolved Hide resolved

jingting1412 reviewed Apr 6, 2024

View reviewed changes

packages/core/src/utils/htmlValidationUtil.ts Outdated Show resolved Hide resolved

packages/core/src/utils/htmlValidationUtil.ts Outdated Show resolved Hide resolved

yiwen101 marked this pull request as draft April 12, 2024 11:34

yiwen101 and others added 3 commits April 17, 2024 23:54

Merge branch 'master' into hydration_rule

a400737

Rewrite

5f2d3d5

Improve docs

98ec18c

yiwen101 marked this pull request as ready for review April 17, 2024 17:09

yucheng11122017 reviewed Apr 18, 2024

View reviewed changes

yiwen101 and others added 5 commits April 18, 2024 11:57

Update docs/devGuide/design/serverSideRendering.md

e8cee88

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update packages/core/test/unit/utils/HtmlValidationUtil.test.ts

4630d35

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update packages/core/src/utils/htmlValidationUtil.ts

186155c

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Improve code quality

6c7ede8

Add newline after the log

843316c

yucheng11122017 reviewed Apr 18, 2024

View reviewed changes

yiwen101 and others added 4 commits April 18, 2024 15:11

Update packages/core/test/unit/utils/HtmlValidationUtil.test.ts

be965f2

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update packages/core/src/utils/htmlValidationUtil.ts

69a1e3a

Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>

Update test

3df4777

Merge branch 'master' into hydration_rule

9df100e

EltonGohJH reviewed Apr 18, 2024

View reviewed changes

packages/core/src/utils/htmlValidationUtil.ts Show resolved Hide resolved

yiwen101 added 3 commits April 18, 2024 17:05

Update testcase

9375e98

Improve code quality

c052a4d

Revert "Improve code quality"

ff5634c

This reverts commit c052a4d.

EltonGohJH approved these changes Apr 18, 2024

View reviewed changes

yucheng11122017 approved these changes Apr 18, 2024

View reviewed changes

yiwen101 merged commit f616fd2 into MarkBind:master Apr 18, 2024
7 checks passed

yiwen101 added r.Patch Version resolver: increment by 0.0.1 r.Minor Version resolver: increment by 0.1.0 and removed r.Patch Version resolver: increment by 0.0.1 r.Minor Version resolver: increment by 0.1.0 labels Apr 18, 2024

This was referenced May 3, 2024

Vue components having block-level elements in <span> #1850

Open

Investigate and implement validation for the most common invalid HTML violations #2076

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule based html validation against hydration #2493

Rule based html validation against hydration #2493

yiwen101 commented Apr 3, 2024 •

edited

codecov bot commented Apr 3, 2024 •

edited

yucheng11122017 left a comment

kaixin-hc commented Apr 5, 2024

jingting1412 left a comment

yiwen101 commented Apr 17, 2024 •

edited

kaixin-hc commented Apr 17, 2024

yucheng11122017 commented Apr 17, 2024

yiwen101 commented Apr 17, 2024

yucheng11122017 left a comment

yucheng11122017 left a comment

EltonGohJH left a comment

EltonGohJH left a comment

yucheng11122017 left a comment

yiwen101 commented Apr 18, 2024

github-actions bot commented Apr 18, 2024

Rule based html validation against hydration #2493

Rule based html validation against hydration #2493

Conversation

yiwen101 commented Apr 3, 2024 • edited

codecov bot commented Apr 3, 2024 • edited

Codecov Report

yucheng11122017 left a comment

Choose a reason for hiding this comment

kaixin-hc commented Apr 5, 2024

jingting1412 left a comment

Choose a reason for hiding this comment

yiwen101 commented Apr 17, 2024 • edited

kaixin-hc commented Apr 17, 2024

yucheng11122017 commented Apr 17, 2024

yiwen101 commented Apr 17, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

EltonGohJH left a comment

Choose a reason for hiding this comment

EltonGohJH left a comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

yiwen101 commented Apr 18, 2024

github-actions bot commented Apr 18, 2024

yiwen101 commented Apr 3, 2024 •

edited

codecov bot commented Apr 3, 2024 •

edited

yiwen101 commented Apr 17, 2024 •

edited