Material-components-web-components: Components don't build properly with PolymerLabs/lit-element-build-rollup

[larsgk]

Reproduce:

• Clone https://github.com/PolymerLabs/lit-element-build-rollup
• Update lit-element to latest (2.1.0)
• Build and verify that build works (it does ;))
• Add a component e.g. npm install @material/mwc-button & import/add in my-element
  (The element version is 0.5.0)
• Clean and build

Two things happen:

1. the build throws an error:

src/index.js → build/index.js...
(!) `this` has been rewritten to `undefined`
https://rollupjs.org/guide/en#error-this-is-undefined
node_modules/@material/mwc-button/mwc-button.js
1: var __decorate = this && this.__decorate || function (decorators, target, key, desc) {
                    ^
2:   var c = arguments.length,
3:       r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc,
...and 1 other occurrence

and 2. the app never renders - but throws an error in the console:

util.js:43 Uncaught TypeError: Cannot read property 'appendChild' of null
    at detectEdgePseudoVarBug (util.js:43)
    at supportsCssVariables (util.js:76)
    at ripple-directive.js:26

Using the rollup script from https://open-wc.org/building/building-rollup.html works with the mwc-elements.

Also, other elements done with lit-element work (with the PolymerLabs/lit-element-build-rollup script).

Theory: The TypeScript compiler creates extra 'boilerplate' code for properties that is not compatible with the simple rollup script.

'captaincodeman' (polymer slack) suggested adding this to the typescript config of the components (I didn't have a chance to try yet):

"noEmitHelpers": true,
"importHelpers": true,

[larsgk]

Related to: https://github.com/PolymerLabs/lit-element-build-rollup/issues/6

[CaptainCodeman]

Just to clear something up ...

I think you have 2 different issues here. The noEmitHelpers and importHelpers prevent the rollup build warnings and also reduce the overall bundle size (decorators imported from tslib instead of repeated in each component).

That's separate to why the app might not be rendering.

[larsgk]

@CaptainCodeman - you are right on the rollup warnings being separate (they also turn up on the open-wc build, I see)

[aomarks]

(Copying my comment from #323 here)

We should set importHelpers: true and add a dependency on tslib so that the helpers like __decorate are imported from that module instead of inlined. This should also help code size and performance, since currently every element has its own duplicate of __decorate (and maybe some other helpers), not to mention most typescript users are also going to need this function too.

(This is arguably also a tsc bug, in that when emitting a module, this and var aren't going to set globals, so not only is there a copy of each helper at every call site, there is no runtime de-duplication via globals.)

[mi-ztroh]

@aomarks When can we expect NPM component releases including this fix? Thanks!

[aomarks]

@aomarks When can we expect NPM component releases including this fix? Thanks!

Just released 0.8.0 with this change in it. Enjoy!

[mi-ztroh]

@aomarks I filed a separate issue against the wicg-inert repo (see above) that appears to be a dependency for certain components. Does it make sense to reopen this issue until the dependency issue is resolved as you'll likely need to update package.json for any affected components?

For example, mwc-drawer won't function within a Rollup bundle until this is resolved.

https://github.com/material-components/material-components-web-components/blob/master/packages/drawer/package.json#L23

[robdodson]

I'm not sure I understand what is causing the bug. @aomarks can you explain it to me?

[aomarks]

I'm not sure I understand what is causing the bug. @aomarks can you explain it to me?

This repo (MWC) uses JavaScript modules, and we import wicg-inert as a JavaScript module (see https://github.com/material-components/material-components-web-components/blob/b1d1640c9c3b27ec0f3c9054c1b2a158f23a891a/packages/drawer/src/mwc-drawer-base.ts#L23).

However wicg-inert is distributed as an AMD module (not a JavaScript module), so it has the standard AMD initialization boilerplate at the top of the file, which includes a reference to a top-level this (see https://unpkg.com/browse/[email protected]/dist/inert.js). That this is usually window and AMD uses it to put things into a global namespace.

In JavaScript modules, top-level this is undefined (not window). In general, it doesn't make sense to import an AMD module as though it were a JavaScript module. In the particular case of wicg-inert, the library doesn't export anything, and is only used for its side-effects, so the this is never actually used. But when Rollup processes this import, it still notices this problem and errors.

IMO the solution here is that wicg-inert should either not be compiled to an AMD module at all (especially since it doesn't export anything), or there needs to be a second JavaScript module output, with the package.json module field set to that output (Rollup and other JavaScript-module aware tools will prefer "module" over "main").

[aomarks]

I'll re-open this until the wicg-inert issue is fixed and we bump the dependency here.

[robdodson]

I think we're distributed as a UMD module so, AMD if it's available, or CommonJS, else global.

I think the second option of using the module field to point at an ES Module version sounds like a plan.

[mi-ztroh]

@aomarks I can confirm the new wicg-inert version works with Rollup bundling. Thanks @robdodson.

[aomarks]

Thanks for fixing and releasing @robdodson! NPM install should already pick up the new version (users may need to delete package-lock.json and node_modules/ first). I also just bumped our own dependency on inert, to ensure the next MWC release will require Rob's fix.

[robdodson]

btw I think this lead to an issue where folks using webpack were expecting a transpiled version to come through in the module field. https://github.com/WICG/inert/issues/136

It seems like there might be a de facto standard of sorts where folks transpile the guts of a module but leave the import/export syntax in place. If I were transpile inert to ES5 but leave the import/export syntax would that negatively affect y'all?

[aomarks]

It seems like there might be a de facto standard of sorts where folks transpile the guts of a module but leave the import/export syntax in place. If I were transpile inert to ES5 but leave the import/export syntax would that negatively affect y'all?

It won't break anything in MWC, but it will probably slightly increase bundle size (since transpiled classes are larger), and it makes the code a little less debuggable in the browser. I don't think this module actually has any imports/exports, it's all side-effects. (But if it did, shipping ES5 transpiled code but with module syntax seems pretty odd, because you don't get the benefit of it working out-of-the-box in older browsers, but you also remove the possibility of serving smaller/faster code to modern browsers).

Sounds like this should have been a breaking change (totally my bad too, since we discussed it and I didn't know about this kind of use case), but TBH I don't think the fact that certain webpack configurations expect the "module" field to contain ES5 code is a compelling reason to shy away from publishing ES2015 to NPM. IMO if a user needs to support older browsers, they should have a build process that transpiles all their code, including dependencies, to ES5.