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

bug (header) It doesn't work on IE11; Add polyfill for CustomEvent to support IE11 #186

Open
tonysaad opened this issue Jul 4, 2018 · 11 comments

Comments

@tonysaad
Copy link
Contributor

tonysaad commented Jul 4, 2018

As we started using some tools for external users ex. Suppliers, it usually comes with the price of supporting browsers we don't even use and that's the old monster IE, ex. IE11
Now if you open any of our tools that use Fabric Header it will be a white page, not rendering anything because there's an error in IE11 support of Custom Event so we need a polyfill which is pretty straightforward or as CanIUse say we can initiate the constructor differently

https://caniuse.com/#feat=customevent

try {
  const ce = new window.CustomEvent('test');
  ce.preventDefault();
  if (ce.defaultPrevented !== true) {
    // IE has problems with .preventDefault() on custom events
    // http://stackoverflow.com/questions/23349191
    throw new Error('Could not prevent default');
  }
} catch (e) {
  const CustomEvent = (event, params) => {
    const evt = document.createEvent('CustomEvent');
    const origPrevent = evt.preventDefault;
    const newParams = params || {
      bubbles: false,
      cancelable: false,
      detail: undefined,
    };
    evt.initCustomEvent(event, newParams.bubbles, newParams.cancelable, newParams.detail);
    evt.preventDefault = () => {
      origPrevent.call(this);
      try {
        Object.defineProperty(this, 'defaultPrevented', {
          get: function () {
            return true;
          },
        });
      } catch (e) {
        this.defaultPrevented = true;
      }
    };
    return evt;
  };

  CustomEvent.prototype = window.Event.prototype;
  window.CustomEvent = CustomEvent; // expose definition to window
}
@tonysaad tonysaad changed the title Add polyfill for CustomEvent to support IE11 bug (header) It doesn't work on IE11; Add polyfill for CustomEvent to support IE11 Jul 4, 2018
@tonysaad tonysaad closed this as completed Jul 4, 2018
@tonysaad tonysaad reopened this Jul 6, 2018
@fragsalat
Copy link
Contributor

I would rather initialize the custom event in a proper way than adding a polyfill just because it's used from one abstract class.
Kind of

let event;
if (typeof window.CustomEvent === 'function') {
  event = new CustomEvent('change', detail, bubbles);
} else {
  event = document.createEvent('CustomEvent');
  event.initCustomEvent('change', bubbles, false, details);
}
this.element.dispatchEvent(event);

I would say this might work even in IE9. At least initCustomEvent exists till this version. document.createEvent I'm not sure. There is no real data in caniuse.com nor in mdn.

@mroderick
Copy link
Contributor

If you want to support legacy IE (it's all legacy), you'll have to use var instead of let/const.

@fragsalat
Copy link
Contributor

My dear Morgan. This ES6/7 will be transpiled :D Times for writing var is over :D
So all the components are compiled into modules using amd, commonjs, es6 and system which are es5 compatible. The settings should even support ie9.

@tonysaad
Copy link
Contributor Author

@fragsalat I agree with you

@tonysaad
Copy link
Contributor Author

Just got a feedback from Arindam that few suppliers aren't using our tools because it's not working on IE at all, I think we should ship this polyfill
any objections from you ? @fragsalat @mroderick @szafranek @fokusferit

@szafranek
Copy link
Contributor

With transpiling to ES5 and polyfills for ancient browsers we punish everyone who uses modern browsers.

So it's a question of whether we'd like to either:

  1. offer better user experience to the vast majority of users, and reduced testing and development cost on our side (I mention this, because using a transpiler is not the same as ensuring that everything works in IE11 – proper testing is still required, if we decide to support it)
    OR
  2. degrade UX and increase our effort, but in return allow a small group of suppliers to use the tool at all (i.e. potentially get new business).

I don't know how big is the potential win from choosing option no. 2. E.g. how many suppliers are we talking about? How much longer will they use IE11?

Sorry for not offering a more specific advice :).

@fragsalat
Copy link
Contributor

fragsalat commented Aug 28, 2018

First of all about which version of ie we're talking here?
I think 10 is possible to support without a lot of tweaks. If you're able to get these number we can investigate for 1 hour if it's possible to fix it and if not we have to talk about proper solutions.
One option could be to build different versions for different browsers. This could mean that we transpile to es5 + polyfill for ie and other older browsers and another build could target ff45+.

PS: Can you send me on hangouts the url of the problematic site?

@mroderick
Copy link
Contributor

I agree that we shouldn't punish everyone that use modern browsers with heavy polyfills that they don't need.

I am also of the opinion that it would be wasteful to do significant investment into what can only be considered a shrinking user base, we should optimise for the present and the future, not the past.

However, browser support doesn't have to be a dichotomy, it can be a scale.

We can use https://polyfill.io/ to make the browsers only download the polyfills that they actually need.

@mroderick
Copy link
Contributor

As for transpiling or not, we can support both legacy and modern browsers fairly simply by offering two versions of JavaScript: one for modern browsers, that understand ESM and an ES5.1 version for browsers that don't.

<script type="module" src="module.mjs"></script>
<script nomodule src="fallback.js"></script>

See https://jakearchibald.com/2017/es-modules-in-browsers/

@fokusferit
Copy link
Contributor

First thing to mention is:

  • In Retail Portal we don‘t use fabric header, it‘s for now vanilla JS
  • in the future most apps will live there -> most of the current logic can be removed or many apps just won‘t have a header anymore

So let‘s rethink effort vs. outcome/benefits here.

So this topic might be more important for Alpha and Portal header then here.

Besides that, I‘d like to understand more which apps are affect and how many suppliers?

Let‘s move this discussion to internal

@tonysaad
Copy link
Contributor Author

Thank you all for your thoughts, just wanted to highlight that Right now ANY application that would use our Fabric Header won't work on IE 11 or less actually, it will be a white page with an error in the console (only works on Edge though)

I know we never cared about IE, and it'd require testing to make sure applications are working but having white page is just the worse thing we can do at least we can show a link to download chrome/firefox https://browser-update.org/

I'm mainly concerned to make sure basic functionality is working and not breaking people's application if they decided to use our header, at least the website is visible and not just using Fabric makes it a white page with errors

@fokusferit Good that you're not using it in Retail Portal, but as for now Web Re-Order is using Fabric Header and consequently it's a white page with zero usability

@fokusferit fokusferit reopened this Aug 30, 2018
@fabric-design fabric-design locked as too heated and limited conversation to collaborators Aug 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants