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

[Polymer] Bundling ES5 shim breaks ES6 elements #3497

Closed
madeleineostoja opened this issue Apr 28, 2018 · 17 comments
Closed

[Polymer] Bundling ES5 shim breaks ES6 elements #3497

madeleineostoja opened this issue Apr 28, 2018 · 17 comments
Labels

Comments

@madeleineostoja
Copy link

Bug or support request summary

@storybook/polymer currently bundles the webcomponents es5 shim (in the <head> of the preview iframe) which makes it impossible to use storybook with untranspiled elements, since the shim throws on ES6 classes. Should leave it to the user to include the shim, and just put a note in the README.

Use cases this breaks:

  • Developing internal tools that don't need to support IE11
  • Developing for evergreens first, and then layering on legacy support later in your workflow (my use case)

FWIW should also probably bundle webcomponents-loader.js rather than webcomponents-lite, which feature detects what polyfills are needed (ie: none on Chrome). Or just not bundle the polyfills either, again leaving it to the user.

Steps to reproduce

  • Setup @storybook/polymer
  • Write story that pulls in ES6 element class
  • ES5 shim throws

Please specify which version of Storybook and optionally any affected addons that you're running

  • @storybook/polymer 4.0.0-alpha.3

Using with @polymer/lit-element (Polymer 3), but same use cases would apply to Polymer 2.x.

Affected platforms

  • N/A
@igor-dv
Copy link
Member

igor-dv commented Apr 28, 2018

@seaneking , do you have a reproduction somewhere?

@madeleineostoja
Copy link
Author

Of the ES5 shim throwing with an ES6 class? Sure, pens:

@igor-dv
Copy link
Member

igor-dv commented Apr 28, 2018

Maybe I am missing something in the problem. In the polymer example here we do have components with es6 syntax (https://github.com/storybooks/storybook/blob/release/3.4/examples/polymer-cli/src/simple-button.html) And everything is working.

@madeleineostoja
Copy link
Author

Is the default webpack config transpiling them?

@igor-dv
Copy link
Member

igor-dv commented Apr 29, 2018

Yes. Also, we have a babel step in it. Do you use an extended webpack config mode?

@madeleineostoja
Copy link
Author

Yeah (typescript), but the same issue would apply if you were to target evergreens only with your Babel config (ie: don't transpile). Same use-cases as outlined in initial issue.

@igor-dv
Copy link
Member

igor-dv commented Apr 29, 2018

I see.
Is custom-elements-es5-adapter a shim?

@madeleineostoja
Copy link
Author

@igor-dv
Copy link
Member

igor-dv commented Apr 29, 2018

You can try to remove this shim from the extended webpack config

 entry: {
      manager: [require.resolve('./polyfills'), managerPath],
      preview: [
        require.resolve('./polyfills'), // <- replace this with a patched one
        require.resolve('./globals'),
        `${require.resolve('webpack-hot-middleware/client')}?reload=true`,
      ],
    },

polyfills.js looks like this:

import '@webcomponents/webcomponentsjs/webcomponents-lite';
import '@webcomponents/webcomponentsjs/custom-elements-es5-adapter'; // <- 
import 'core-js/es6/symbol';
import 'core-js/fn/array/iterator';
import 'airbnb-js-shims';
import 'babel-polyfill';

you need to replace the polyfills.js with your own that imports everything (or only what you need) from the above but not the custom-elements-es5-adapter

@madeleineostoja
Copy link
Author

madeleineostoja commented Apr 30, 2018

Ah okay awesome. Is that documented somewhere? Must have missed it, tried to look for a way to do that before making this issue. If not, would be good to note it in the @storybook/polymer readme.

Not sure how those polyfills are treated in storybook, but worth noting that the es5 shim cannot be transpiled, or it breaks.

FWIW still think the shim should be unbundled before 4.0 of the Polymer setup goes public, since it's a destructive 'polyfill' (webcomponentsjs/webcomponents-loader doesn't automatically apply the shim, for example).

@madeleineostoja
Copy link
Author

madeleineostoja commented Apr 30, 2018

Tried this (had to update to alpha.4 for that setup), but now I can't get it to build, something in storybook's Babel config throwing, don't have time to debug it atm.

@igor-dv
Copy link
Member

igor-dv commented Apr 30, 2018

Is that documented somewhere

This particular extension is not documented. But extending webpack config in common is documented, so it depends on the users' imagination of how to handle this =). But you are welcome to PR it to docs 😉

FWIW still think the shim should be unbundled before 4.0 of the Polymer setup goes public, since it's a destructive 'polyfill' (webcomponentsjs/webcomponents-loader doesn't automatically apply the shim, for example).

I can't advocate about this since hardly familiar with the polymer adoption / best practices, so would be nice to hear the pros and cons, and what actually people do.

Tried this (had to update to alpha.4 for that setup), but now I can't get it to build, something in storybook's Babel config throwing, don't have time to debug it atm.

Mind share the error ?

CC. @stijnkoopal, if you have any thought.

@madeleineostoja
Copy link
Author

depends on the users' imagination of how to handle this

Haha I think that's a liberal use of 'imagination', since to fix it you'd have to dig through the src of @storybook/polymer to figure out how it's being polyfilled. Case in point, it changed from alpha.3 to alpha.4 without a breaking change warning, so I'm guessing it's not considered public interface.

I can't advocate about this since hardly familiar with the polymer adoption / best practices, so would be nice to hear the pros and cons, and what actually people do.

Fair enough - IMO it's generally an antipattern to bundle polyfills, especially when those polyfills aren't passive, ie: they require trade-offs or cause incompatibilities, which is certainly the case with the shim (not so much with webcomponentsjs). It's a nasty hack that is unfotunately required for transpiled classes, but it's destructive and fragile. But as long as there's an easy way to opt-out I guess it's not a big deal.

Oh and FWIW not specific to Polymer, all this applies to web components in general (Stencil, SkateJS, etc).

Mind share the error ?

Sorry false alarm, nuking my node_modules seems to have fixed it. Can't get my stories working for some reason anyway, and I suspect it's to do with webpack-polymer-loader not being friendly with js / polymer 3 elements. Giving up it for now 🤷‍♂️

@stale
Copy link

stale bot commented May 21, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label May 21, 2018
@stale
Copy link

stale bot commented Jun 20, 2018

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

@stale stale bot closed this as completed Jun 20, 2018
@jackblackCH
Copy link

This has not been solved yet, a story can't be created from a simple ES6 class, which prevents also the usage of other features like lit-html

@web-padawan
Copy link
Contributor

@jackblackCH let's continue discussion in the new issue #5531 which describes potential fix.

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

No branches or pull requests

5 participants