Skip to content
This repository has been archived by the owner on Apr 15, 2022. It is now read-only.

Conflicts with React 0.14 #29

Closed
mnpenner opened this issue Mar 7, 2016 · 12 comments
Closed

Conflicts with React 0.14 #29

mnpenner opened this issue Mar 7, 2016 · 12 comments
Labels

Comments

@mnpenner
Copy link

mnpenner commented Mar 7, 2016

This library causes select.onchange (and possibly other) React events to not fire in IE8. There was a ticket open over there, but they've closed it because they're dropping IE8 support starting with v0.15. I'm not sure if this can be fixed or not on this (ie8.js) end, but it'd be nice 😄

@WebReflection
Copy link
Owner

I've read the ticket and yet I've no idea what is exactly the problem.

Do you include this polyfill before React?

Do you include dom4 too before React?

If so, which version of both ie8 and dom4 ?

if not, can you give me the smallest possible example that involves the smallest amount of React code in order to fail?

In case we are going to solve this, what's the point since React won't support IE8 anyway?

Thanks.

@mnpenner
Copy link
Author

mnpenner commented Mar 8, 2016

  • Yes, I included this polyfill before React
  • I'm not using DOM4
  • I'm using v0.3.2 of ie8

Here's a pretty short snippet. Do you need the HTML too?

I won't be able to upgrade React until my users get off IE8 like they promised (probably not for a couple more years!) so for the time being I'll be sticking with React 0.14.x.

@WebReflection
Copy link
Owner

I like how easily you blamed ie8.js in that ticket.

So here the thing, ie8.js just works like it should.

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <!--[if IE 8]><script src="//cdnjs.cloudflare.com/ajax/libs/ie8/0.3.2/ie8.js"></script><![endif]-->
    <script>
    this.onload = function () {
      'use strict';

      var app = document.getElementById('my-app');
      var select = app.appendChild(document.createElement('select'));
      for (var option, options = ['a', 'b', 'c'], i = 0; i < options.length; i++) {
        option = select.appendChild(
          document.createElement('option')
        );
        option.setAttribute('value', options[i]);
        option.textContent = options[i];
      }
      select.addEventListener('change', function (e) {
        alert('changed');
      });
    };
    </script>
  </head>
  <body><div id="my-app"></div></body>
</html>

Above page doesn't generate any error, it does what it's supposed to do and it doesn't require all the framework code and logic.

This is a non issue per ie8.js because if you create a select with few options and a listener, this just works.

Sorry but:

  • I'd like you to clarify in there ie8.js had nothing to do with this problem I've done it for ya
  • I'd like you to consider not using frameworks that don't support IE8 or you gonna be here many times

This polyfill has been used to normalize the DOM as much as possible and it does the job so I'll close this as bogus.

Best Regards

@mnpenner
Copy link
Author

mnpenner commented Mar 9, 2016

Getting a little defensive there, eh?

My React select worked just fine as soon as I removed this single line from my code:

<!--[if IE 8]><script src="//cdnjs.cloudflare.com/ajax/libs/ie8/0.3.2/ie8.js"></script><![endif]-->

Now I'm not saying that ie8.js doesn't "work as it should", I'm simply saying that it conflicts with React 0.14.x, exactly like my title says. No one's arguing that it doesn't work with regular (non-React) <selects>. It might be ie8.js's fault, it might be React's fault, I don't know, all I'm saying is that they don't play nicely together and I was hoping you could fix it on your end, but that doesn't seem so likely now.

I'd like you to consider not using frameworks that don't support IE8 or you gonna be here many times

React 0.14 explicitly supports IE8.

Since its 2013 release, React has supported all popular browsers, including Internet Explorer 8 and above. We handle normalizing many quirks present in old browser versions, including event system differences, so that your app code doesn't have to worry about most browser bugs.

React might not support IE8 future, but as of now (until v15 is released) it does, so I intend to stay on this version.

Here's a full example if you need further proof that there's a conflict:

N.B. React does require a shim which could also be the source of the problem.

@WebReflection
Copy link
Owner

You see how easy it is to provide more details, examples to tests, and make a point?

Although all I am saying is that as soon as you put React 0.14 in the page ie8.js doesn't work anymore.

I don't understand why would you use i8.js if you have a framework that works already with IE8.

This project is about bringing standards to IE8, you can use ES5 shim and sham without problems, you can add dom4.js without problems, you can add dom-class withotu problems, based on document-register-elements ... you can have all the standards you can dream of in IE8.

Instead, you want to go non standards and put a framework on top that is planning to drop IE8 support soon and that works already so I am not sure this is going to be fixed anywhere because:

  • ain't nobody got time for that
  • pick your weapon: standards or React 0.14?
  • I can easily debug ie8.js because it's a relatively little script, I've got no idea, neither is my role, neither IE8 makes it easy, what the hell happens in React 0.14 and why it wouldn't triggar ergular events.

If you can help finding out what is the problem that makes React conflict then I'll have a look.

@WebReflection
Copy link
Owner

P.S. about being defensive, this script that took "forever" to be created and tested has been blamed instead of considering that maybe the problem comes in some other part of the stack. These kind of comments (your first one there and the way you filed the bug here providing pretty much zero details, no playground, pointing finger and not much more) aren't helpful for the OSS. At least you got it right with the latest comment, let's see if we can together get this sorted out in a way or another.

@WebReflection
Copy link
Owner

If react and fbjs won't accept my two PRs I'll have to manually release a patched version of React.

As Summary

React gives for granted that if there is an addEventListener it means it's safe to use it and ignore completely the whole logic used in place in order to make IE8 work.

My patch informs React 0.14 when it's appropriate to use addEventListener and when it's not, making this or any other tests I've made safe to run in IE8 without compromising any other browser.

Let's hope this will be merged and that ie8.js won't be blamed for patching as it can the IE8 DOM.

Libraries around are supposed to do better feature detection than just "addEventListener" in document if they are meant to be used against IE8 too because this is not the only repository that fixes somehow IE8 but obviously not everything can work as native modern browser would.

@mnpenner
Copy link
Author

mnpenner commented Mar 9, 2016

I'm still not sure what you're so upset about, all you had to do was ask for some more details. It takes time to put together a self-contained snippet. If the original React ticket provided enough detail for you to diagnose the problem then great -- it would save me some time -- if not, just say you need more. I never "blamed" ie8.js or accused you of writing bad code, I just said there was a conflict with the two libraries and I didn't know on which end it needed to be fixed. If there's nothing you could do on your end, all you would have to do is say so; I'm not familiar with the internals of either library to be able to make that call.

I'm not sure what your "standards or React" argument is all about either. My project isn't pure React. It's an old, large codebase and I was hoping to maximize support but utilizing your little library here. Someone over in the webpack chat suggested it.

Anyway, thank you for submitting a PR.

@WebReflection
Copy link
Owner

I've over-React-ed and I apologies for that but the reason is quite simple: React does a very naive check about addEventListener assuming that if it's there no IE8 logic is needed anymore.

So, React does a runtime check about ("addEventListener" in document) or target.addEventListener every single time it needs to add an event instead of checking this once per its entire lifetime like any other library or framework would do.

This means fragile results because as soon as some little library does document.addEventListener = function (t,h) {this.attachEvent("on"+t,h)} the mighty React "dies" .

As result, people believe smaller less famous libraries are the cause (this is in the initial ticket where people started saying that this or that was breaking) instead of thinking that maybe, even if a library is famous and comes from a famous company, there could be some, in this case obious, problem with the library, not those surrounding.

Specially in my case I write a lot of polyfills and I couldn't imagine doing a single runtime check per native functionality or such weak feature detection at runtime and per each event ... it makes absolutely no sense to me yet my code, or the code mentioned in that ticket, gets the blame.

Do you see why someone that had to push 2 PR in another repository to avoid conflicts nobody apparently cares in there (closed ticket indeed) might get a bit upset reading stuff like:

I first tried with this one. However, it broke React's onChange event handling. I guess it is overwriting too much of the original js methods. I tried the official polyfill at MDN. That didn't work either.
...
I just encountered this problem and it was due to ie8.js

So nope, nope, and nope ... React is naive and highly exposed to corrupted logic because it does way too fragile checks and at runtime, assuming that target.addEventListener means the browser is not IE8 (but v 0.14 says it supports it, right?)

I hope they'll be reasonable enough to either accept my PRs and move on, or change the way they feature detect addEventListener-ability because as it is it's like telling people every other script beside react-oriented scripts is not welcome.

I'm a former FB engineer and I know for sure there are reasonable developers in there so let's see how it goes, I'm confident it'll be solved in a way or another.

I hope I have at least explained a bit better my initial React-ion (dehihi)

@mnpenner
Copy link
Author

mnpenner commented Mar 9, 2016

No problem. I know it sucks to have your hard work crapped on, especially when you've done everything right.

@WebReflection
Copy link
Owner

I cant do more than this.

facebook/fbjs#126 (comment)

If they won't accept the change in their 0.14, I'm afraid you'll have to use my hand made patch that works like a charm and better than current stable.

I'm waiting for them to be reasonable about this, I really don't want to waste to much time convincing them they are doing it wrong.

@WebReflection
Copy link
Owner

told'ya there are reasonable developers in there, indeed there's an even better solution that will work using ie8.js upfront.

facebook/react#3131 (comment)

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

No branches or pull requests

2 participants