Skip to content

Conversation

@syranide
Copy link
Contributor

@syranide syranide commented May 8, 2014

One possible fix for #1494 and at the same time enabling the safe use of non-visible tags in IE8 for React components as well.

The down-side is cost, http://jsperf.com/testnoscrip, in worst-case scenarios it's 20% slower, that is; there's no leading text and only one element being inserted. However, using a more precise test (with no hits) the overall performance seems to only dip by 6%. Looking at http://jsperf.com/testnoscrip/3 with 6 elements being inserted, the cost of the lookups has already become reduced to only 2% slower overall performance. This does NOT include any overhead by React nor layouting/rendering, so the numbers should be significantly lower in practice. Also, this obviously only affects IE8.

Another possibility is to tag HTML insertions of non-visible tags inside React which could probably remove the overhead all-together, but I don't think there's an obvious non-intrusive way of doing it in React that would be worth it considering only IE8 is affected.

My PR implements the faster fix, including another check intended to further improve certain cases.

   raw     gz Compared to master @ 32b84a4c5ea32835b93a857cf00f0db86d6c755a
     =      = build/JSXTransformer-previous.js
     =      = build/JSXTransformer.js
     =      = build/react-previous.min.js
 -1477   +141 build/react-test.js
  +569   +184 build/react-with-addons.js
  +148    +45 build/react-with-addons.min.js
  +569   +181 build/react.js
  +148    +46 build/react.min.js

@syranide
Copy link
Contributor Author

syranide commented May 8, 2014

cc @chenglou @zpao

@chenglou
Copy link
Contributor

chenglou commented May 8, 2014

that's neat lol, 👍

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zpao
Copy link
Member

zpao commented Jun 19, 2014

@chenglou can you get the right people looking at this and get it reviewed and landed?

@syranide
Copy link
Contributor Author

@chenglou Rebased, no conflicts. I don't know if there are any practical implications, but using <link> or <meta> instead would have the benefit of being inherently self-closing tags (not that it really matters in practice size-wise).

@chenglou
Copy link
Contributor

@syranide diff created, although I can't repro this. I also vaguely remember being able to repro it, weird...

@syranide
Copy link
Contributor Author

@chenglou It happens if there is no text before the "invisible" tag. You can try it with just plain innerHTML to see the effect.

@chenglou
Copy link
Contributor

Reproed! Merged internally.

chenglou added a commit to chenglou/react that referenced this pull request Jun 23, 2014
chenglou added a commit that referenced this pull request Jun 23, 2014
Fix ReactEmptyComponent disappearing and throwing in IE8
@chenglou chenglou merged commit f81e213 into facebook:master Jun 23, 2014
chenglou added a commit that referenced this pull request Jun 23, 2014
@syranide syranide deleted the ie8noscript branch June 24, 2014 07:00
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.

4 participants