Skip to content

Resolving implicit thead can lead to invariant violation #2005

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

Closed
syranide opened this issue Aug 6, 2014 · 6 comments
Closed

Resolving implicit thead can lead to invariant violation #2005

syranide opened this issue Aug 6, 2014 · 6 comments

Comments

@syranide
Copy link
Contributor

syranide commented Aug 6, 2014

http://jsfiddle.net/9Q2m2/

Internet is more or less unavailable for me right now, so can't test more thoroughly. But thinking about it, it seems that the "blind-jumps" we do now to detect and resolve implicitly added elements (mainly thead) appears to not actually be safe as-is. Personally I feel like the solution is to not resolve implicitly added elements at all, but rather tell the user of his/her error instead and have them fix it.

Related PR #1987 (Provide a friendly path for reactID errors in ReactMount)

@syranide
Copy link
Contributor Author

syranide commented Aug 6, 2014

cc @zpao @spicyj

@sophiebits
Copy link
Collaborator

Yes, that's why the error tells you to add tbody. :) The current solution works in some cases and I'd rather not make things more strict until we can make them better (i.e., error consistently at an earlier time à la #101).

@syranide
Copy link
Contributor Author

syranide commented Aug 6, 2014

We can error at an earlier time already, we just need to remove the "smart code". We will never be able to error earlier than that, the only way to error earlier than that is with immediate traversal (in dev only perhaps, we can do that today if we want), but even that is not 100% fool-proof for all edge-cases I'm pretty sure (if the DOM is detached or later moved).

Personally I would prefer that it errors as soon as possible, rather than as late as possible. The harder it is for an error to surface, the more likely it is to make it into PROD.

@sophiebits
Copy link
Collaborator

I agree in principle but am also wary of causing unnecessary churn, especially if it turns out we can add the tbody automatically in the future, which I would like to do. I am fine with adding a dev-only warning() call now telling you to add a tbody though.

@syranide
Copy link
Contributor Author

syranide commented Aug 8, 2014

In some ways I'm aware of the "service" we would be providing if we made tbody optional, but it is invalid HTML and it breaks expectations as CSS-rules still have to consider the tbody element.

But regardless of where I stand on that, I don't see how we could ever provide a reliable implementation. There are so many edge-cases, including some that we simply cannot account for. It's a nice idea to transparently add a tbody to return (<table><tr><td></td></tr></table>), but return (<tr><td></td></tr>) and it is a lot less obvious what is going to happen. Throw toElement, {children} and composite components into the mix and everything starts becoming really unintuitive.

<table><thead>...</thead><tr><td></td></tr></table>
<table><tbody>...</tbody><tr><td></td></tr></table>
<table><tr><td></td></tr><thead>...</thead></table>

The only lenient and robust solution I can imagine is only accepting <table><tr><td></td></tr></table> as a special-case, but I don't think the broken expectations are worth it. Had tables been common-place it could definitely be an important "feature", but tables are so rare that I don't see why we should break with the standard. It's also super-easy to create a custom-class that returns <table><tbody>{children}</tbody></table> too if for some reason you require tons of tbody-only tables.

@gaearon
Copy link
Collaborator

gaearon commented Oct 1, 2017

The fiddle doesn’t work for me anymore, and last comment was several years ago.
I’ll close this as stale.

We do warn about bad nesting now, so hopefully this should not be a problem.

@gaearon gaearon closed this as completed Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants