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

version 5.1.1 contains an arrow function #99

Closed
khamiltonuk opened this issue Oct 7, 2020 · 4 comments · Fixed by #106 or #105
Closed

version 5.1.1 contains an arrow function #99

khamiltonuk opened this issue Oct 7, 2020 · 4 comments · Fixed by #106 or #105

Comments

@khamiltonuk
Copy link

I am trying to use the library in internet explorer 11

in the transpiled version there is an arrow function
I can see it coming from line 49

let tabbableNodes = orderedTabbables
    .sort(sortOrderedTabbables)
    .map((a) => a.node)
    .concat(regularTabbables);

https://github.com/focus-trap/tabbable/blob/master/src/index.js#L49

We have two options, we can use a traditional function expression in the index.js, or we can configure the babel settings to have a target of internet explorer 9 to match the version support in the docs

@stefcameron
Copy link
Member

stefcameron commented Oct 7, 2020

@khamiltonuk Thanks for filing this issue! It ended-up being brought-up over on focus-trap/focus-trap#173 and we had a discussion there.

Bringing that over to here now:


Webpack can be configured to work around this. The issue is you're wanting to use ESM as a modern module system, but still expecting to have old browser-compatible code inside those modules, and not wanting to transpile any third-party modules. Is that correct (just trying to understand the needs)?

It feels strange to have diverging targets in the same bundle: ES modules (which is technically ES6) and ES5 JS. But I admit I see the same issue in my bundles that reference focus-trap if I set my Babel target to ie: "11" (I end-up with the same issue with the arrow function @khamiltonuk pointed out earlier).

Looks like this is a problem for a lot of libraries, and Webpack is keeping its position on prioritizing module over main for 3 years now: webpack/webpack#5756 (still open)

A lot of packages seem to have solved this by adding a browser entry to their package.json (because Webpack's default order is mainFields: ['browser', 'module', 'main']), which is something we could do with focus-trap and tabbable.

Apparently, this is the browser field spec, but it doesn't really get into specifics about the nature of the referenced files.

Still, given its name "browser", it feels like it should point to the UMD build.

Thoughts?


👉 I just tried this locally with tabbable's package.json having "browser: dist/index.umd.js" (with Webpack) and the resulting bundle that includes tabbable look nice. Webpack took the right UMD "path" for CJS and plopped the result right into the bundle:

}, function(module, exports, __webpack_require__) {
  !function(exports) {
    "use strict";
    var candidateSelectors = [ "input", "select", "textarea", "a[href]", "button", "[tabindex]", "audio[controls]", "video[controls]", '[contenteditable]:not([contenteditable="false"])', "details>summary" ], candidateSelector = candidateSelectors.join(","), matches = "undefined" == typeof Element ? function() {} : Element.prototype.matches || Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;
    function tabbable(el, options) {
      var regularTabbables = [], orderedTabbables = [];
      return getCandidates(el, (options = options || {}).includeContainer, isNodeMatchingSelectorTabbable).forEach((function(candidate, i) {
        var candidateTabindex = getTabindex(candidate);
        0 === candidateTabindex ? regularTabbables.push(candidate) : orderedTabbables.push({
          documentOrder: i,
          tabIndex: candidateTabindex,
          node: candidate
        });
      })), orderedTabbables.sort(sortOrderedTabbables).map((function(a) {           // <- NO ARROW!
        return a.node;
      })).concat(regularTabbables);
    }

    ...

    exports.focusable = focusable, exports.isFocusable = isFocusable, exports.isTabbable = isTabbable, 
    exports.tabbable = tabbable, Object.defineProperty(exports, "__esModule", {
      value: !0
    });
  }(exports);

Based on this, I believe the correct solution here is not to transpile ESM into ES5 while still keeping import/export statements that are part of the language syntax of ES6 (diverging targets for the same build), but rather to simply add the browser: ./dist/index.umd.js entry to package.json.

stefcameron added a commit that referenced this issue Oct 7, 2020
Fixes #99

AFAICT, this seems to be the best solution to what __a lot__ of folks
in the community have been experiencing with ESM bundles, referenced
from a package's `module` entry, that aren't also transpiled down
to ES5 (which is technically in conflict with ESM, which is an ES6
feature).

The problem arises because this package states both `module` and
`main`, but Webpack defaults `mainFields` to
`['browser', 'module', 'main']`, in order. So it will always pick
`module` over `main` by default, because it doesn't understand
that the build's target is NOT a browser that supports ESM...

See webpack/webpack#5756
stefcameron added a commit that referenced this issue Oct 7, 2020
Fixes #99

AFAICT, this seems to be the best solution to what __a lot__ of folks
in the community have been experiencing with ESM bundles, referenced
from a package's `module` entry, that aren't also transpiled down
to ES5 (which is technically in conflict with ESM, which is an ES6
feature).

The problem arises because this package states both `module` and
`main`, but Webpack defaults `mainFields` to
`['browser', 'module', 'main']`, in order. So it will always pick
`module` over `main` by default, because it doesn't understand
that the build's target is NOT a browser that supports ESM...

See webpack/webpack#5756
stefcameron added a commit that referenced this issue Oct 7, 2020
Fixes #99

AFAICT, this seems to be the best solution to what __a lot__ of folks
in the community have been experiencing with ESM bundles, referenced
from a package's `module` entry, that aren't also transpiled down
to ES5 (which is technically in conflict with ESM, which is an ES6
feature).

The problem arises because this package states both `module` and
`main`, but Webpack defaults `mainFields` to
`['browser', 'module', 'main']`, in order. So it will always pick
`module` over `main` by default, because it doesn't understand
that the build's target is NOT a browser that supports ESM...

See webpack/webpack#5756
@stefcameron
Copy link
Member

@all-contributors add @khamiltonuk for bug

@allcontributors
Copy link
Contributor

@stefcameron

I've put up a pull request to add @khamiltonuk! 🎉

stefcameron added a commit that referenced this issue Oct 7, 2020
Fixes #99

See webpack/webpack#5756

The initial approach was to add a `browser` target to package.json and
point it at the UMD build so that bundlers would pick that up instead of
the ESM build, since they (except for Rollup) tend to prefer `browser`
first, then `module` (ESM), then `main`.

But that would potentially affect tree-shaking, and we don't want to
hamper that optimizing process.

So this PR transpiles the code in the ESM bundle down to ES5, so that
it runs in IE9+ like the CJS and UMD bundles do, while keeping the
import/export ESM statements.
@stefcameron
Copy link
Member

OK, folks, this is hopefully finally fixed in [email protected]. Please LMK if there's still an issue, and my apologies for the hiccup!

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