-
Notifications
You must be signed in to change notification settings - Fork 245
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
ES6 Module #164
Comments
+1 I'm using lit-element and try to use native ES6 modules for making import resources. |
I tend to agree, as I am a huge fan of ES6 modules. I'll see what I can do. Of course, the horrible option is to wrap it in a file like this: import './dialog-polyfill.js';
const dialogPolyfill = window.dialogPolyfill;
export default dialogPolyfill;
delete window.dialogPolyfill; 🤣 |
@samthor would you accept a PR for es module support? Happy to work on this if it's a welcome addition. I'd probably use rollup to create something like |
@mreinstein atleast for me that would be a very welcome addition :D If possible you could put it up on unpkg until they decide what they want to do |
Yes, we're very happy to receive PRs. :) Some of the odd way this code works (the global) is leftover from the very first release. |
Awesome! I'll put together a PR tonight or tomorrow and send it over. |
sorry, I was hoping to send this over to you sooner. Open for feedback/suggestions/code review on the changes! |
I'm very much a bundler noob but it's my understanding that default exports are controversial https://twitter.com/ericsimons40/status/1090771410492960768?s=19 |
I don't know, I can't really speak to that tweet. It sounds like he was burned on export defaults in some library in some way. It lacks specifics. Maybe it is controversial? I'm not sure. I know that rollupjs sets this up by default: https://rollupjs.org/guide/en#importing ( scroll slightly to default import) |
Some complaints about default exports are listed at https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad , but these are mostly surmountable with linting rules. Since a polyfill really has one purpose, it would seem unlikely to me that one would wish to cram other items into the same polyfill, so I personally don't see anything wrong with default exports here. However, for the PR, I'd recommend that the source use an ES6 export since Rollup allows one to indicate via standard means that this is a module by design (without committing to a specific variable name, except in the global build). (There are still advantages for having a separate ESM distribution build though, as it allows distributions all in the same directory and if any dependencies are ever needed, the ESM dist will get these bundled.) Adding the export to source also avoids the need for |
We don't have opinions on default exports vs not. Like I said above, the way the polyfill works is mostly left over. Others I've worked on use @brettz9 is mostly right though, please ES6 export and we can build to CJS for the NPM release. (I'll look at the PR again tomorrow). |
I've taken another stab at this, simplifying where possible. Here's the current state of things:
Let me know if this is missing anything or has problems. I'll close this PR and send a clean one when we're confident this is ready to merge. |
I might suggest making a separate |
Can we leave comments on the PR? #176 |
support esm, cjs modules #164 and remove bower support
this issue should be resolved now since the PR was merged. @samthor can we get a new npm release for this please? 🙏 |
As a continuation of this, if you load in const dialogPolyfill = require('dialog-polyfill'); If you bundle this down with webpack, it defaults to resolving the As a fix for this, would it be possible to support the "exports": {
".": {
"import": "./dist/dialog-polyfill.esm.js",
"require": "./dist/dialog-polyfill.js"
}
}, I did this in a test project and then if you did |
actually now that the last remaining version of node that lacks support for esm will be unmaintained in April (soon) https://nodejs.org/en/about/releases/ it might make sense to drop everything but esm, bump the major version. |
It'd be nice to have this as an ES6 module (as well as UMD) to avoid polluting the HTML.. Thanks!
The text was updated successfully, but these errors were encountered: