added useAddEventListener helper to avoid conflicts in patched IE8

[WebReflection]

In order to solve this issue, which ended up raising this bug, and since the future version of React won't support IE8 but developers stuck in 0.14 would like to keep using it and also move on with the browser, this push aim is to resolve conflicts with patched IE8 environments, either via my ie8.js library or others, so that React 0.14 won't wrongly assume that if there is an addEventListener, that's what should be used as if it's native.

[WebReflection]

dunno about that babel failures for babel-6/__tests__/dev-expression.js

I don't think it has anything to do with this PR

cat babel-6/__tests__/dev-expression.js
/**
 * Copyright (c) 2015, Facebook, Inc. All rights reserved.
 *
 * This source code is licensed under the BSD-style license found in the
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
 */

'use strict';

let babel = require('babel-core');
let devExpression = require('../dev-expression');

function transform(input) {
  return babel.transform(input, {
    plugins: [devExpression],
  }).code;
}

function compare(input, output) {
  var compiled = transform(input);
  expect(compiled).toEqual(output);
}

var oldEnv;

describe('dev-expression', function() {
  beforeEach(() => {
    oldEnv = process.env.NODE_ENV;
    process.env.NODE_ENV = '';
  });

  afterEach(() => {
    process.env.NODE_ENV = oldEnv;
  });

  it('should replace __DEV__ in if', () => {
    compare(
`
if (__DEV__) {
  console.log('foo')
}`,
`
if (process.env.NODE_ENV !== 'production') {
  console.log('foo');
}`
    );
  });

  it('should replace warning calls', () => {
    compare(
      `warning(condition, 'a %s b', 'c');`,
      `process.env.NODE_ENV !== 'production' ? warning(condition, 'a %s b', 'c') : undefined;`
    );
  });

  it('should replace invariant calls', () => {
    compare(
      `invariant(condition, 'a %s b', 'c');`,
      `!condition ? process.env.NODE_ENV !== 'production' ? invariant(false, 'a %s b', 'c') : invariant(false) : undefined;`
    );
  });
});

[WebReflection]

same exact code landed in react branch and not a single error was generated:
facebook/react#6225

I have no idea what is this build error about. Please let me know if there's anything I should do to fix this.

Thanks.

[sophiebits]

This would be a behavior change in IE9 and IE10 to use attachEvent instead of addEventListener, correct?

[WebReflection]

I'll double check but AFAIK not at all. IE9 dropped non standard events
already.
On Mar 9, 2016 6:59 PM, "Ben Alpert" notifications@github.com wrote:

This would be a behavior change in IE9 and IE10 to use attachEvent instead
of addEventListener, correct?

—
Reply to this email directly or view it on GitHub
#126 (comment).

[WebReflection]

I've commented latest push in here:
facebook/react#6225 (comment)

Travis got stuck, hope this can go through at some point.

Best Regards

[zpao]

No idea why the build is failing but I just added some logging to see what error we are getting, so you might want to rebase on top of that. f0243de

[WebReflection]

thanks. Right now Travis is fully stuck so I'm not committing or pushing anything until it starts, at least, my latest push.

Let's hope for the best.

[sophiebits]

Best practice for polyfills is to include them before libraries so every library sees a consistent view of the available APIs. For example, including web components polyfills after React causes strange errors and is not recommended by us or them.

Do you know what the actual issue here is? addEventListener works well for React in modern browsers and attachEvent works in old IE – but it sounds like addEventListener with ie8.js does not work as expected. What's the behavioral difference that causes this?

[WebReflection]

it's the other way round. ie8.js works like a charm, I even brought CustomElements in IE8 and modern dom4 standards.

React 0.14 has a very weak and naive feature detection, performed at runtime every single time.

As soon as any little script one this one

// all it takes to break React 0.14 any time in E8
document.addEventListener = function (type, handler) {
  this.attachEvent('on' + type, handler);
};

would completely screw any React logic.

No wonder including polyfill after React causes problems, indeed all I am pushing here is a way to make your logic bullet proof instead of assuming that if "addEventListener" in document or target.addEventListener is there it means that all your capturing events will work just fine.

There is the problem: your poor, runtime, each time, feature detection assumes a polyfill can make miracles and it doesn't guard, nor grant, consistent user experience as soon as somebody write a shortcut like the one i've already mentioned.

Your team put a warning for the case there's no addEventListener and you are setting events through attachEvent, which is indeed what hanppens to React 0.14 as soon as a polyfill goes in.

TL;DR

1. this change won't affect this library, it will actually make it more robust and less prone to shenanigans caused by good, aweome, or very bad polyfills ... why would you care?
2. this change forces developers to actually run your code before other polyfills so that it will work as it should
3. only IE8 will be affected by this change, not a single other platform
4. not fixing this here means React 0.14, that is also stopping supporting IE8 in its next release, doesn't want developers to be able to upgrade IE8 to more modern standards because as it is, React 0.14 is against IE8 polyfills since there's no bullet proof solution for capturing pphase and native, IE8 souce, events logic.

This library should grant itself to be consistent with the environment ... at least do it once, and trust yiur first feature detection instead of breaking at runtime for no concrete reason wharsoever.

I hope I've explained why it is importante that Reac 0.14 won't make itself impossible to be used for those abandoned developers forced to support IE8 for still a while.

Thanks for your understanding.

[WebReflection]

To bring even more food on my plate, I've commented this library file I'm trying to make more robust

  /**
   * Listen to DOM events during the capture phase.
   *
   * @param {DOMEventTarget} target DOM element to register listener on.
   * @param {string} eventType Event type, e.g. 'click' or 'mouseover'.
   * @param {function} callback Callback function.
   * @return {object} Object with a `remove` method.
   */
  capture: function (target, eventType, callback) {
    if (target.addEventListener) {
      target.addEventListener(eventType, callback, true);
      //                             see this up here?
      //                            that won't just work
      //                            in your capture native logic
      //                            indeed, if you check down there
      //                            look what you think about
      //                            using attachEvent to simulate
      //                            the capture phase
      return {
        remove: function () {
          target.removeEventListener(eventType, callback, true);
        }
      };
    } else {
      if (process.env.NODE_ENV !== 'production') {
        //                          yep, this is exactly the situation
        //                          any polyfill is trying to avoid
        //                          or patch as best as it could.
        //
        //                          The point is, React 0.14
        //                          works in IE8 because it uses
        //                          attachEvent logic and will never
        //                          end up here.
        //                          If you don't sandbox once your
        //                          very quick and dirty feature detection
        //                          no IE8 polyfill could possuibly ever work
        //                          as meant by this library.
        console.error(
          'Attempted to listen to events during the capture phase on a ' +
          'browser that does not support the capture phase. Your application ' +
          'will not receive some events.');
      }
      return {
        remove: emptyFunction
      };
    }

Hoping it's clear why you cannot blame any IE8 polyfill for failing within the logic that your own team flagged as inappropriate.

They are right, using capture in a non capture capable browser is a very bad idea.

Best Regards

[sebmarkbage]

See facebook/react#6225 Thanks!