Skip to content

Validate node nesting structure #644

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
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Collaborator

This solution feels a little gross to me, but I haven't been able to come up with anything better.

Fixes #101.

This solution feels a little gross to me, but I haven't been able to come up with anything better.

Fixes facebook#101.
@sophiebits
Copy link
Collaborator Author

I ran react-bench and there was no significant perf difference relative to master.

@jordwalke
Copy link
Contributor

It's been a while since I've had context to this particular problem. Here's a couple of thoughts:

  1. We try to strive to make dev and production be functionally equivalent - modulo any difference in the amount of information logged. So it's good that you chose to log a warning instead of an error.
  2. With this diff, we allocate twice as many objects upon the initial render (and then use pooled instances going forward) (compared to the number of strings we allocate/return from the mounting process). But we only perform the validation on these new objects in DEV. Couldn't we simply use a regex to test the leading few characters of every markup fragment we insert? This would help us avoid having to pay the cost of all the allocations (or getting pooled instances) in production at all. And even if we feel the regex based check is hacky, it's just something that runs in DEV anyways, so we don't have to feel bad about it.

@sophiebits
Copy link
Collaborator Author

Yeah, that makes sense and is simpler for sure. I considered it before I implemented it. This seemed cleaner before I implemented it; but it turns out that this way is a lot of code! I was sort of hoping that the MountImage class could store more info in the future so you could check descendant nesting (for nested <a> tags) and things like that but it's overkill for now.

@jordwalke
Copy link
Contributor

@spicyj - I hear you on the "cleaner" part. In fact, I like this much better. But for the web, at least, we'll probably always want the "image" to be either a chunk of markup, or a single DOM node (when node creation becomes faster in all browsers).

@vjeux
Copy link
Contributor

vjeux commented Dec 28, 2013

What's the status here? Do we want to pursue this diff?

@sophiebits
Copy link
Collaborator Author

I'll close it for now.

@sophiebits sophiebits closed this Dec 28, 2013
sophiebits added a commit to sophiebits/react that referenced this pull request Feb 12, 2014
This is a better version of facebook#644. Fixes facebook#101.
sophiebits added a commit to sophiebits/react that referenced this pull request Mar 20, 2015
Nicer version of facebook#644 and facebook#735. Fixes facebook#101. Context is neat.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React should throw on nested <p> tags
3 participants