Skip to content

Use faster cached regexps for tests in setInnerHTML#1885

Merged
zpao merged 1 commit intofacebook:masterfrom
syranide:testinnerhtml
Jul 31, 2014
Merged

Use faster cached regexps for tests in setInnerHTML#1885
zpao merged 1 commit intofacebook:masterfrom
syranide:testinnerhtml

Conversation

@syranide
Copy link
Copy Markdown
Contributor

This is significantly faster than the current code.
http://jsperf.com/reacttestindexof (only relevant on IE8)

Also added comments to the list of non-visible tags and made the test even stricter.

Since this code is explicitly aimed at IE8, we could also switch the current extensive "feature test" for just document.documentMode === 8, it correctly identifies emulation mode (which emulates the crappy whitespace behavior). The upside to this would be that we only target IE8 mode and doesn't apply potentially wonky logic to some other potentially broken browser out there (which I doubt there is).

@zpao
Copy link
Copy Markdown
Member

zpao commented Jul 21, 2014

Seems like a reasonable thing to take as is. I actually want to figure out the longer term IE8 plan as it would be great to be able to continue supporting it in some way but also build modern browser builds (your polyfill all the features idea is interesting).

@sophiebits
Copy link
Copy Markdown
Collaborator

if (__IE8__)

@syranide
Copy link
Copy Markdown
Contributor Author

@zpao The polyfill is interesting, but there are two flaws I think:

  1. May mess with other libraries that incorrectly assume features are available based on unrelated feature testing, I don't think it is an issue, but it could be for some obscure library.
  2. It doesn't shim other documents (i.e. iframes), which is a significant flaw. It's possible to somewhat workaround in various ways, but the unavoidable issue is... should you really force the shim onto other documents? I think the answer is no... although this only really affects events, so it could still be interesting/practical.

I'm currently experimenting with how to support IE8 (and possibly other less capable browsers) for our service (we use iframes a lot) without making it too invasive or feeling like too much of a commitment. What I'm doing right now is simply just defining helper utils for all the properties/functions that I want and put them conditionally behind an if-statement.

For instance:

var pageXOffset = getPageXOffset(window);

Which is simply implemented as:

var getPageXOffset;
if (!__supportLegacyBrowsers__ || 'pageXOffset' in window) {
  getPageXOffset = function(win) {
    return win.pageXOffset;
  };
} else {
  getPageXOffset = function(win) {
    return win.document.documentElement.scrollLeft;
  };
}

This creates a bit of overhead as uglifyjs isn't smart enough to inline these for the modern browser build, but for React core, the callsites for legacy features are so few and far in-between that we could just write:

var pageXOffset = __supportOldBrowsers__ ?
  oldBrowserUtil.getPageXOffset(window) :
  window.pageXOffset;

// feel free to use if-statements if this triggers a gag reflex :)

__supportOldBrowsers__ ?
  oldBrowserUtil.setInnerHTML(target, html) :
  target.innerHTML = html;

The minimal overhead of require('oldBrowserUtil') will always be there though, unless we also put that behind a condtional.

Since the fix is contained within oldBrowserUtil the callsites themselves would never have to be fixed. There would be minimal overhead as it could all be contained within a single module. So it would be equally maintainable, just a little more verbose.

@syranide
Copy link
Copy Markdown
Contributor Author

@zpao #1901

zpao added a commit that referenced this pull request Jul 31, 2014
Use faster cached regexps for tests in setInnerHTML
@zpao zpao merged commit ab2406f into facebook:master Jul 31, 2014
@syranide syranide deleted the testinnerhtml branch July 31, 2014 19:14
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.

3 participants