Skip to content

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