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

Doesn't work in Microsoft Edge (EdgeHTML 18) #1023

Closed
te1 opened this issue Jun 29, 2020 · 3 comments · Fixed by #1026
Closed

Doesn't work in Microsoft Edge (EdgeHTML 18) #1023

te1 opened this issue Jun 29, 2020 · 3 comments · Fixed by #1026

Comments

@te1
Copy link
Contributor

te1 commented Jun 29, 2020

Open https://shepherdjs.dev/ in Microsoft Edge and you can't access the demo because of a syntax error in the script.
SCRIPT1028: SCRIPT1028: Expected identifier, string or number

The problem is the ...popperOptions object spread in the code below which is not supported in the (old) EdgeHTML 18 version.
https://caniuse.com/#feat=mdn-javascript_operators_spread_spread_in_object_literals

Your browserlist specifies last 2 Edge versions which apparently doesn't include Edge 18 anymore (see Can I Use) but according to https://browserl.ist/?q=last+2+Edge+versions it still does. I'm not sure what to make of this.

Transpilation should fix the problem, so including Edge 18 in your browserlist should be enough. Alternatively maybe the documentation should explicitly state support for "Chromium Edge" only. The problem with that is that according to Can I Use EdgeHTML has a global usage of 2% and Chromium Edge is still at 0%.

export function makeCenteredPopper(step) {
  const centeredStylePopperModifier = _getCenteredStylePopperModifier();

  let popperOptions = {
    placement: 'top',
    strategy: 'fixed',
    modifiers: [
      {
        name: 'focusAfterRender',
        enabled: true,
        phase: 'afterWrite',
        fn() {
          setTimeout(() => {
            if (step.el) {
              step.el.focus();
            }
          }, 300);
        }
      }
    ]
  };

  popperOptions = {
    ...popperOptions,
    modifiers: Array.from(
      new Set([...popperOptions.modifiers, ...centeredStylePopperModifier])
    )
  };

  return popperOptions;
}
@RobbieTheWagner
Copy link
Member

@te1 I think Chromium Edge would be the only supported type of Edge. We only want to support the last couple versions. We dropped IE as well, and non-Chrome Edge I think is going to fall into the same category of not being supported. That being said, if adding Edge 18 does not increase the bundle size much, I wouldn't mind adding it, we just wouldn't be officially supporting anything but the last 2 versions.

@te1
Copy link
Contributor Author

te1 commented Jul 1, 2020

The difference in JS bundle size is very minor. As I don't have Java installed the Closure Compiler part didn't work for me. CSS order seemed changed for me but the content was the same.

Current

+--------------------------------------+
¦                                      ¦
¦   Destination: dist/js/shepherd.js   ¦
¦   Bundle Size:  158.26 KB            ¦
¦   Minified Size:  49.42 KB           ¦
¦   Gzipped Size:  15.93 KB            ¦
¦                                      ¦
+--------------------------------------+
+------------------------------------------+
¦                                          ¦
¦   Destination: dist/js/shepherd.esm.js   ¦
¦   Bundle Size:  153.26 KB                ¦
¦   Minified Size:  59.13 KB               ¦
¦   Gzipped Size:  17.29 KB                ¦
¦                                          ¦
+------------------------------------------+

With Edge 18

+--------------------------------------+
¦                                      ¦
¦   Destination: dist/js/shepherd.js   ¦
¦   Bundle Size:  158.66 KB            ¦
¦   Minified Size:  49.63 KB           ¦
¦   Gzipped Size:  15.99 KB            ¦
¦                                      ¦
+--------------------------------------+
+------------------------------------------+
¦                                          ¦
¦   Destination: dist/js/shepherd.esm.js   ¦
¦   Bundle Size:  153.65 KB                ¦
¦   Minified Size:  59.36 KB               ¦
¦   Gzipped Size:  17.35 KB                ¦
¦                                          ¦
+------------------------------------------+
+	function _extends() {
+	  _extends = Object.assign || function (target) {
+	    for (var i = 1; i < arguments.length; i++) {
+	      var source = arguments[i];
+
+	      for (var key in source) {
+	        if (Object.prototype.hasOwnProperty.call(source, key)) {
+	          target[key] = source[key];
+	        }
+	      }
+	    }
+
+	    return target;
+	  };
+
+	  return _extends.apply(this, arguments);
+	}
+

-	  popperOptions = { ...popperOptions,
+	  popperOptions = _extends({}, popperOptions, {
 	    modifiers: Array.from(new Set([...popperOptions.modifiers, ...centeredStylePopperModifier]))
-	  };
+	  });

@RobbieTheWagner
Copy link
Member

@te1 would you like to open a PR adding it? That bundle size difference seems fine to me.

@te1 te1 closed this as completed Jul 2, 2020
@te1 te1 reopened this Jul 2, 2020
RobbieTheWagner pushed a commit that referenced this issue Jul 2, 2020
-Enables transpilation of object property spread
-Required for (old) EdgeHTML 18 compatibility
-Fixes #1023
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.

2 participants