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

React warning for whitespace in tables #28

Closed
stevemk14ebr opened this issue Jun 15, 2021 · 10 comments · Fixed by #29
Closed

React warning for whitespace in tables #28

stevemk14ebr opened this issue Jun 15, 2021 · 10 comments · Fixed by #29
Labels
💪 phase/solved Post is done

Comments

@stevemk14ebr
Copy link
Contributor

stevemk14ebr commented Jun 15, 2021

I'm using the plugin like this:

 const markdownConverter = Remark().use([RemarkParse, RemarkGFM,
        [RehypeUnwrap, { disallowedElements: ["p", "pre"], unwrapDisallowed: true }],
        [RehypeReact, { createElement: React.createElement, Fragment: React.Fragment }]]);
const htmlmarkdown = markdownConverter.processSync(markdown).result;

Given the following markdown:

"|File Name|File Type|Size|Compile Time|\n| ----------- | ----------- | ----------- | ----------- |\n|nothing|nothing|nothing|nothing|"

The generated table has a bunch of newline text nodes as children, which causes react to throw the error "Whitespace text nodes cannot appear as a child of < tbody >" and more for different type like tr and th.

Example of the spurious nodes:
image

I'm not sure how to troubleshoot this or disable these

@stevemk14ebr stevemk14ebr added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jun 15, 2021
@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jun 15, 2021

@stevemk14ebr a few things:

  1. you need remark-rehype to turn markdown/mdast into html/hast
  2. Not sure what rehype unwrap is or where it came from, it doesn't appear to be on npm

When used with remark rehype and without the unwrap plugin no error is present.
https://codesandbox.io/s/remark-rehype-debug-forked-dtzkw?file=/src/index.js

@ChristianMurphy ChristianMurphy added 🤷 no/invalid This cannot be acted upon and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jun 15, 2021
@stevemk14ebr
Copy link
Contributor Author

stevemk14ebr commented Jun 16, 2021

Hi your demo is using remark-stringify which does not appear to have this issue. As far as i can tell the issue is with rehype-react. I am using remark-rehype just forgot to include that in the snippet, rehype unwrap is a custom thing i made it's not relevant. Here is a reproduction of what i'm seeing, can this be re-opened please?

https://codesandbox.io/s/remark-rehype-debug-forked-21j7e

interestingly the issue here is not just \n like i was seeing. There's quite a few empty text nodes at each level of the tree:
image

@ChristianMurphy
Copy link
Member

Thanks for clarifying!
I'm not sure this is coming from rehype-react
inspecting the AST at each step, the spaces appear to come into existence after remark-rehype
https://codesandbox.io/s/rehypereact-bug-forked-sjuse

@ChristianMurphy ChristianMurphy added 🧘 status/waiting and removed 🤷 no/invalid This cannot be acted upon labels Jun 16, 2021
@stevemk14ebr
Copy link
Contributor Author

oh cool thanks for showing how to dump the tree easily! I agree it does seem to be the case, it seems like it's trying to format the html it generates nicely by including some extra newlines and whitespace but these nodes are not removed and so cause issues like this. It seems these ARE present with the stringify plugin too, however since you set the html directly and bypass react the whitespace errors are not present (as react's parser generates these)

@wooorm
Copy link
Member

wooorm commented Jun 16, 2021

The newlines are supposed to be there at the remark-rehype phase. React for some unnecessary reason doesn’t like newlines though.
A PR similar to remarkjs/remark-react@228fd8e for this project would be a solution

@wooorm wooorm added 🙆 yes/confirmed This is confirmed and ready to be worked on and removed 🧘 status/waiting labels Jun 16, 2021
@wooorm wooorm changed the title Invalid newline nodes React warning for whitespace in tables Jun 16, 2021
@stevemk14ebr
Copy link
Contributor Author

So that I understand, the remark-hype plugin used to not emit these newlines but that was removed a while ago? Now, this rehype-react plugin should be responsible for removing these newlines (since they're only invalid in this context)?

@wooorm wooorm reopened this Jun 16, 2021
@wooorm
Copy link
Member

wooorm commented Jun 16, 2021

No, remark-rehype has always emitted them, and it adheres to how markdown works: with newlines.
The fact that React warns about them, is more an issue with React.
For some reason, in most cases, React does not care about the newlines, but in certain cases it does warn. I’m not exactly what your setup is to cause the warning, but indeed: we can remove them in this plugin

@stevemk14ebr
Copy link
Contributor Author

it's the table type for me that causes issues, but i'm sure react cares about other html types

wooorm pushed a commit that referenced this issue Jun 23, 2021
Closes GH-28.
Closes GH-29.

Reviewed-by: Titus Wormer <tituswormer@gmail.com>
@wooorm wooorm added ⛵️ status/released and removed 🙆 yes/confirmed This is confirmed and ready to be worked on labels Jun 23, 2021
@wooorm
Copy link
Member

wooorm commented Jun 23, 2021

@wooorm wooorm added the 💪 phase/solved Post is done label Aug 7, 2021
@TheComp44
Copy link
Contributor

I think the issue should be reopened, since the fix doesn't work for all whitespace nodes. Just filtering for "\n" is too specific as whitespace nodes could also consist of an arbitrary number of newlines, spaces and tabs.

When using rehype-parse on an html document with a table, I get the same react warning since the test misses on tables with indentations. For example:

<table>
    <tbody>
        <tr>
            <td>Test</td>
        </tr>
    </tbody>
</table>

wooorm pushed a commit that referenced this issue Sep 10, 2021
Reviewed-by: Titus Wormer <tituswormer@gmail.com>

Related-to GH-28.
Related-to GH-29.
Closes GH-32.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

4 participants