Skip to content

FYI: Single out IE8 as a separate execution environment#1901

Closed
syranide wants to merge 1 commit intofacebook:masterfrom
syranide:condie8
Closed

FYI: Single out IE8 as a separate execution environment#1901
syranide wants to merge 1 commit intofacebook:masterfrom
syranide:condie8

Conversation

@syranide
Copy link
Copy Markdown
Contributor

https://github.com/facebook/react/pull/1901/files?w=1 (without whitespace changes)

@zpao I did a quick test just to see what would happen if we singled out IE8 and made it possible to strip it from the final build. I'm pretty confident that no other old browser got caught in the cross-fire, but would obviously have to validate (not that I recommend taking this PR as-is), and I think there are a few other call sites that are IE8 specific. I was also unsure about getMarkupWrap, couldn't quite decode what was IE8 specific and not.

Current master size:

649770 133520 build/react-with-addons.js
135443  36996 build/react-with-addons.min.js
591846 121396 build/react.js
125593  34400 build/react.min.js

IE8 compatible build (can be made smaller):

  +569    +55 build/react-with-addons.js
  +452    +77 build/react-with-addons.min.js
  +569    +59 build/react.js
  +452    +79 build/react.min.js

IE9+ compatible build (isIE8 is constant false):

  -273    -36 build/react-with-addons.js
 -1338   -407 build/react-with-addons.min.js
  -273    -38 build/react.js
 -1338   -405 build/react.min.js

So it stripped away slightly more than 1% (from master), mostly courtesy of 3 large functions being stripped out of ChangeEventPlugin + innerHTML I assume. Regardless of if we care about separate builds, the method of using isIE8 in this PR might still be worth switching to (cssShorthandExpansion is no longer applied to all browsers, etc).

PS. It also seems like ViewportMetrics should be able to be put under isIE8.

@syranide syranide changed the title Cond IE8 FYI: Single out IE8 as a separate execution environment Jul 21, 2014
@zpao
Copy link
Copy Markdown
Member

zpao commented Aug 14, 2014

I think we're not going to do this. It doesn't play super nicely with other UA detection and is a bit out of place. I would rather continue feature detection in place for the time being. Feel free to leave the branch around if you think we should come back to it.

We can talk about the CSS part of it in your other diff.

@zpao zpao closed this Aug 14, 2014
@syranide
Copy link
Copy Markdown
Contributor Author

I don't necessarily disagree with you on UA detection, my primary motivation behind this is that only IE<9 should these specific quirks with these specific fixes (I sincerely doubt that x.replaceNode(y, y) will fix for any another potentially broken browser out there).

That being said, this was mostly an experiment and I don't feel particularily strongly about this. I think the idea is sound, but practically it shouldn't amount to any real benefit, but it has quite a wide surface area where there is some potential for unexpected failure. So definitely fine with leaving it as-is 👍

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.

2 participants