Skip to content
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

Minified version broken in IE8 #2247

Closed
cascadian opened this issue Sep 26, 2014 · 21 comments
Closed

Minified version broken in IE8 #2247

cascadian opened this issue Sep 26, 2014 · 21 comments

Comments

@cascadian
Copy link

line 66 of setInnerHTML node.innerHTML = '\uFEFF' + html; is minified as e.innerHTML = "" + t; in react.min.js and react-with-addons.min.js. An exception is thrown on on line 71 because textNode.data is undefined.

Here's a repro that's modified from the basic example

    <script src="../../build/react.min.js"></script>
    <script>
      var Container = React.createClass({
        render: function(){
          return React.DOM.div(null, this.props.children);
        }
      });
      var ExampleApplication = React.createClass({
        render: function() {
          var elapsed = Math.round(this.props.elapsed  / 100);
          var seconds = elapsed / 10 + (elapsed % 10 ? '' : '.0' );
          var message =
            'React has been successfully running for ' + seconds + ' seconds.';

          return React.DOM.p(null, message);
        }
      });
        React.renderComponent(
          Container(null,
            ExampleApplication({elapsed: new Date().getTime()}),
          document.getElementById('container')
        ));

    </script>
@sophiebits
Copy link
Collaborator

What encoding are you serving your HTML with? My guess is if you add <meta charset="utf-8"> to your <head> then this will work correctly, though maybe we should use only ASCII chars in our minified builds.

@sophiebits
Copy link
Collaborator

Never mind, I see that the character is actually missing. Maybe a bug in uglifyjs?

@syranide
Copy link
Contributor

@spicyj As it's encoded the charset is irrelevant unless uglifyjs decodes it, being zero-width/invisible I imagine it would look like nothing is there... but it really seems like nothing is there. Very weird.

@syranide
Copy link
Contributor

https://github.com/mishoo/UglifyJS2/blob/master/lib/parse.js#L209

Is apparently the culprit, but it doesn't seem like it should be. Perhaps uglifyjs decodes strings first and then strips for some reason...

@syranide
Copy link
Contributor

mishoo/UglifyJS#556

@syranide
Copy link
Contributor

Hmm, this is interesting, this problem only surfaces if you double-uglify the source, perhaps something is wrong in our build-process?

@syranide
Copy link
Contributor

PR #2249

@syranide
Copy link
Contributor

@cascadian I should add; if this is a priority for you simply replace .innerHTML="" with .innerHTML="!" (you can replace "\uFEFF" with "!" in the source too) and it should work.

@zpao
Copy link
Member

zpao commented Sep 29, 2014

this problem only surfaces if you double-uglify the source, perhaps something is wrong in our build-process

Yes, we double uglify to remove dead requires from the tree (see #1933).

@dennis-johnson-dev
Copy link
Contributor

Is this still an issue? I'm seeing it with react-with-addons.min.js v 0.12.2 in IE8.

@syranide
Copy link
Contributor

syranide commented Jan 4, 2015

Ping, this is resolved in uglifyjs/master now at least. React should go the way of #2340 if you ask me though.

@markplindsay
Copy link

This issue, although easily fixed by @syranide's DIY patch, still exists in http://fb.me/react-0.13.1.min.js and any other FB-packaged react.min.js. :| Sorry to be a nag. In the end this is all IE8's fault anyway.

@syranide
Copy link
Contributor

@markplindsay I haven't verified, but this was fixed upstream in UglifyJS some time ago so it should have solved itself... @zpao is it possible you're running the build process with an older UglifyJS?

@markplindsay
Copy link

@syranide, thanks for your followup. I am looking at a react-0.13.1.min.js I wgot from http://fb.me/react-0.13.1.min.js. Changing e.innerHTML="" to e.innerHTML="!" by adding the ! at Line 15, Column 17852 still fixes the issue.

@syranide
Copy link
Contributor

@zpao Ah, apparently we've locked the uglifyjs dependency to 2.4.0 whereas the fix is in 2.4.17 (or perhaps the one after it, not sure what the tag means in mishoo/UglifyJS@24bc09b). There's no reason not to update the dependency right?

@mockdeep
Copy link

We just got bit by this issue. Please update uglify!

@sophiebits
Copy link
Collaborator

AFAIK no reason not to upgrade. @syranide want to send a PR to the 0.13-stable branch? (I forget how to update shrinkwrap files properly though…)

@zpao
Copy link
Member

zpao commented Apr 10, 2015

Send to master and I'll pull it in with other things

cheers,
Paul

On Apr 10, 2015, at 12:26 PM, Ben Alpert <[email protected]mailto:[email protected]> wrote:

AFAIK no reason not to upgrade. @syranidehttps://github.com/syranide want to send a PR to the 0.13-stable branch? (I forget how to update shrinkwrap files properly though…)


Reply to this email directly or view it on GitHubhttps://github.com//issues/2247#issuecomment-91658188.

zpao added a commit to zpao/react that referenced this issue Apr 13, 2015
Apparently we needed to get to v2.4.17 to get the fix for facebook#2247. We
shrinkwrapped on the same day but the timing didn't work out so we missed it.
@zpao
Copy link
Member

zpao commented Apr 13, 2015

Just confirmed that this is correct in the minified build now. We'll ship 0.13.2 this week with the fix.

@zpao zpao closed this as completed Apr 13, 2015
@ledowong
Copy link

will 0.12.x get rebuild too?

@sophiebits
Copy link
Collaborator

@ledowong No, we don't generally do point releases except for the newest minor release except in the case of a security vulnerability. You can patch in the change locally and make your own build of 0.12 if you're unable to upgrade to 0.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants