Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate DOM nesting when possible #3310

Open
karlhorky opened this issue Jun 15, 2022 · 11 comments
Open

Validate DOM nesting when possible #3310

karlhorky opened this issue Jun 15, 2022 · 11 comments

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Jun 15, 2022

Hey @ljharb, long time no see! 👋 Have been pretty inactive around here, sorry about that! Lots of new students.

Wanted to see what you think about a new feature to validate DOM nesting when possible, to avoid such errors as seen below - which now, as of React 18, lead to hydration errors.

Problem: th is not a valid child of tfoot

export default function IndexPage() {
  return (
    <div>
      <table>
        <tfoot>
          <th>abc</th> {/* 💥 Error: Hydration failed because the initial UI does not match what was rendered on the server. */}
        </tfoot>
      </table>
    </div>
  );
}

I remember React did some kind of simple validation of DOM nesting, although I haven't seen this error for a while now (maybe just haven't run into it any more).

Maybe eslint-plugin-react could have some simple JSX validation (maybe based on a well-maintained library). Not across component boundaries or anything, only simple React elements which are HTML tags.

It leads to this (pretty confusing) error with latest Next.js (see this StackBlitz):

Screen Shot 2022-06-15 at 16 44 45

If you were to open the DevTools and scroll up all the way to the top, you would see a more helpful error message (which, arguably, React should also display), but it's pretty hidden. Also, it's a confusing error message for those unfamiliar with the finer details of HTML or how React creates nodes:

Screen Shot 2022-06-15 at 16 45 15

@ljharb
Copy link
Member

ljharb commented Jun 16, 2022

I think that this would be very useful IFF we had a shared data source between React and this plugin, that matched what browsers accepted. Do you know of one?

@karlhorky
Copy link
Contributor Author

There's this file from the React source here: https://github.com/facebook/react/blob/main/packages/react-dom/src/client/validateDOMNesting.js (apparently Sophie extracted a subset of the spec, Dan mentioned)

Maybe that plus some additional rules for common things?

@ljharb
Copy link
Member

ljharb commented Jun 17, 2022

Is that reachable from the actual npm package?

@karlhorky
Copy link
Contributor Author

karlhorky commented Jun 17, 2022

Seems like validateDOMNesting and findInvalidAncestorForTag are exposed in react-dom@18.2.0/cjs/react-dom.development.js (although not explicitly exported)

@ljharb
Copy link
Member

ljharb commented Jun 17, 2022

If we can access them from react, that’d be sufficient, but we’d have to be able to do so across many react versions :-/

@karlhorky
Copy link
Contributor Author

karlhorky commented Feb 26, 2023

Ah looks like there is an ESLint plugin from @MananTank that does this over here 👀

This is based on validate-html-nesting, also by @MananTank. The src/mapping.js file looks like a distillation of the validateDOMNesting rules.

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 8, 2023

Workaround

Using some of the original validateDOMNesting code from @sophiebits and the src/mapping.js file from @MananTank's validate-html-nesting, I was able to come up with a simple set of rules to prevent most footguns for our students using ESLint's no-restricted-syntax rule (uses @typescript-eslint/utils for the config type at the first line):

(This has also been released as part of @upleveled/eslint-config-upleveled@3.13.0)

.eslintrc.cjs

/** @type {import('@typescript-eslint/utils').TSESLint.Linter.Config} */
const config = {
  rules: {
    'no-restricted-syntax': [
      'warn',
      // Warn on nesting <a> elements, <button> elements and framework <Link> components inside of each other
      {
        selector:
          "JSXElement[openingElement.name.name='a'] > JSXElement[openingElement.name.name=/^a|button|Link$/]",
        message:
          'Invalid DOM Nesting: anchor elements cannot have anchor elements, button elements or Link components as children',
      },
      {
        selector:
          "JSXElement[openingElement.name.name='button'] > JSXElement[openingElement.name.name=/^a|button|Link$/]",
        message:
          'Invalid DOM Nesting: button elements cannot have anchor elements, button elements or Link components as children',
      },
      {
        selector:
          "JSXElement[openingElement.name.name='Link'] > JSXElement[openingElement.name.name=/^a|button|Link$/]",
        message:
          'Invalid DOM Nesting: Link components cannot have anchor elements, button elements or Link components as children',
      },

      // Warn on nesting of non-<li> elements inside of <ol> and <ul> elements
      {
        selector:
          "JSXElement[openingElement.name.name=/^ol|ul$/] > JSXElement[openingElement.name.name!='li'][openingElement.name.name!=/^[A-Z]/]",
        message:
          'Invalid DOM Nesting: ol and ul elements cannot have non-li elements as children',
      },

      // Warn on nesting common invalid elements inside of <p> elements
      {
        selector:
          "JSXElement[openingElement.name.name='p'] > JSXElement[openingElement.name.name=/^div|h1|h2|h3|h4|h5|h6|hr|ol|p|table|ul$/]",
        message:
          'Invalid DOM Nesting: p elements cannot have div, h1, h2, h3, h4, h5, h6, hr, ol, p, table or ul elements as children',
      },

      // Warn on nesting any invalid elements inside of <table> elements
      {
        selector:
          "JSXElement[openingElement.name.name='table'] > JSXElement[openingElement.name.name!=/^caption|colgroup|tbody|tfoot|thead$/][openingElement.name.name!=/^[A-Z]/]",
        message:
          'Invalid DOM Nesting: table elements cannot have element which are not caption, colgroup, tbody, tfoot or thead elements as children',
      },

      // Warn on nesting any invalid elements inside of <tbody>, <thead> and <tfoot> elements
      {
        selector:
          "JSXElement[openingElement.name.name=/tbody|thead|tfoot/] > JSXElement[openingElement.name.name!='tr'][openingElement.name.name!=/^[A-Z]/]",
        message:
          'Invalid DOM Nesting: tbody, thead and tfoot elements cannot have non-tr elements as children',
      },

      // Warn on nesting any invalid elements inside of <tr> elements
      {
        selector:
          "JSXElement[openingElement.name.name='tr'] > JSXElement[openingElement.name.name!=/th|td/][openingElement.name.name!=/^[A-Z]/]",
        message:
          'Invalid DOM Nesting: tr elements cannot have elements which are not th or td elements as children',
      },
  },
};

module.exports = config;

This handles the things we see that students most commonly need:

  1. Enforcing nesting rules for:
    • table, thead, tbody, tfoot, tr, th, td
    • ol, ul, li
  2. Warning against common nesting errors with:
    • p and common elements which are not allowed
    • nesting a elements, button elements and framework Link components inside of each other

Looks like this on some invalid code:

Screenshot 2023-03-08 at 19 22 22

Screenshot 2023-03-08 at 19 21 59

@ljharb
Copy link
Member

ljharb commented Mar 8, 2023

That's a great POC for a new rule! I'd love to see a PR that adds it, with lots of test cases :-D

@adeleke5140
Copy link

@ljharb Hello, I notice this PR is still open. what is required to get it to completion and merged into the package?

I ran into some invalid dom nesting errors recently and if eslint can catch that before runtime checks, it would be great.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2023

@adeleke5140 this isn’t a PR, so what’s required is “a PR” :-)

@karlhorky
Copy link
Contributor Author

@adeleke5140 if you'd like to create a PR for this to get it into eslint-plugin-react, that would be amazing!

Here are some potentially useful links:

  1. Our simple version using no-restricted-syntax rules here: https://github.com/upleveled/eslint-config-upleveled/blob/8042b1ff73c1f7d322f80caf796257a4df86cb69/index.cjs#L66
  2. Much more comprehensive eslint-plugin-validate-jsx-nesting by @MananTank

FirePrometheus0109 added a commit to FirePrometheus0109/eslint-config-upleveled that referenced this issue Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants