Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Bug: Compiled dist/mdc.drawer.js file drops ternary expression from tabbable package, giving SSRed apps "ReferenceError: Element is not defined" #4104

Closed
4cm4k1 opened this issue Nov 21, 2018 · 6 comments
Labels

Comments

@4cm4k1
Copy link

4cm4k1 commented Nov 21, 2018

What MDC Web Version are you using?

v0.41.1 via MDC Web React v0.7.0

What browser(s) is this bug affecting?

N/A

What OS are you using?

macOS 10.14.2

What are the steps to reproduce the bug?

  1. Use @material/drawer@^0.41.0 in a server-side rendered app.
  2. Attempt to compile or render the app.

What is the expected behavior?

The code in dist/mdc.drawer.js should have the same ternary expression as found in the tabbable package here: https://github.com/davidtheclark/tabbable/blob/master/index.js#L14. It checks if typeof Element is undefined, as it would be outside of the browser (eg, on Node).

var matches = typeof Element === 'undefined'
  ? function () {}
  : Element.prototype.matches || Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;

(The maintainer of tabbable introduced this logic in v3.1.1 with this PR: focus-trap/tabbable#33)

What is the actual behavior?

What is actually in dist/mdc.drawer.js at line 809 removes the ternary expression as follows:

var matches = Element.prototype.matches || Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;

This causes an error on the server side, as follows:

ReferenceError: Element is not defined
    at Object._ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:352:317)
    at __webpack_require__ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:148:27)
    at Object._ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:761:496)
    at __webpack_require__ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:148:27)
    at Object._ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:325:641)
    at __webpack_require__ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:148:27)
    at Object._ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:258:1499)
    at __webpack_require__ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:148:27)
    at /Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:157:16
    at /Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:157:72
    at webpackUniversalModuleDefinition (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:139:176)
    at Object.<anonymous> (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:142:276)
    at Object.<anonymous> (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:773:30)
    at __webpack_require__ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:30:30)
    at Object.<anonymous> (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:107:1183)
    at __webpack_require__ (/Users/4cm4k1/Code/website/node_modules/@material/react-drawer/dist/index.js:30:30)

Any other information you believe would be useful?

I suspect this has to do with the configuration of bundling everything together as UMD modules in this PR (#3245), which changed the "main" entry for packages to dist/*.js. That is, something in the bundling process is thinking it can save some LOC by eliminating code that isn't relevant to a browser target?

Also #929.

@abhiomkar
Copy link
Collaborator

Thanks for reporting this issue 4cm4k1@!

I'm able to reproduce this issue, seems like the tabbable version that focus-trap depends on is not yet updated. The PR that you referenced is on tabbable@3.1.1 but focus-trap still uses tabbable@3.1.0.

So here is the dependency graph with their versions:
tabbable@3.1.0 <== focus-trap@3.0.0 <== focus-trap-react@4.0.1

I just sent a PR on focus-trap, the similar thing needs to be done for focus-trap-react after focus-trap new release.

@4cm4k1
Copy link
Author

4cm4k1 commented Nov 29, 2018

Cool! Thanks @abhiomkar!

@4cm4k1
Copy link
Author

4cm4k1 commented Nov 29, 2018

Oh, I just noticed and thought I should also point out that this issue will occur with @material/dialog as well because currently the focus-trap version is set to ^2.3.0: https://github.com/material-components/material-components-web/blob/master/packages/mdc-dialog/package.json#L27

Also, if this helps, I think @moog16 changed @material/react-drawer to depend on just focus-trap and not focus-trap-react in the latest version: material-components/material-components-web-react#437

@alexanderjeurissen
Copy link

@4cm4k1 I think you are right in that we won't need to get focus-trap/focus-trap#69 merged upstream in focus-trap-react.

However as it currently stands it seems that the issue originates in @material/drawer/dist/mdc.drawer.js:809 not in the @material/react-drawer code, so my thinking is that the root cause lies in the focus-trap library that is referenced by @material/drawer

relevant stack trace:

/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:809
var matches = Element.prototype.matches || Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;
              ^

ReferenceError: Element is not defined
    at Object.105 (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:809:15)
    at __webpack_require__ (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:35:30)
    at Object.71 (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:2548:16)
    at __webpack_require__ (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:35:30)
    at Object.104 (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:747:69)
    at __webpack_require__ (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:35:30)
    at Object.102 (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:385:64)
    at __webpack_require__ (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:35:30)
    at /Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:78:18
    at /Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:81:10
    at webpackUniversalModuleDefinition (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:8:20)
    at Object.<anonymous> (/Users/alexander/Dev/open_source/snappy.services/node_modules/@material/drawer/dist/mdc.drawer.js:15:3)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
^

I just pinged focus-trap to inquire if they can create a new release that includes the aforementioned PR that got merged in 30 days ago.

@alexanderjeurissen
Copy link

alexanderjeurissen commented Jan 1, 2019

Based on focus-trap/focus-trap#73 it seems that focus-trap @ 4.0.0 contains the changeset of focus-trap/focus-trap#69

So all that remains to be done is to bump the focus-trap definition in the package.json of the mdc-drawer package and get it included in the next release.

@4cm4k1
Copy link
Author

4cm4k1 commented Jan 1, 2019

Great news! Looking forward to it.

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

3 participants