ES6 Module

[brettz9]

It'd be nice to have this as an ES6 module (as well as UMD) to avoid polluting the HTML.. Thanks!

[jukbot]

+1 I'm using lit-element and try to use native ES6 modules for making import resources.

[samthor]

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;

🤣

[mreinstein]

@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 dist/dialog.esm.js, dist/dialog.cjs.js, dialog.global.js (for es modules, commonjs, global script respectively.)

[arjunyel]

@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

[samthor]

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.

[mreinstein]

Awesome! I'll put together a PR tonight or tomorrow and send it over.

[mreinstein]

sorry, I was hoping to send this over to you sooner. Open for feedback/suggestions/code review on the changes!

[arjunyel]

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

[mreinstein]

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)

[brettz9]

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 use strict in source, since modules have this mode on by default (and Rollup will add the strict directive in non-ESM builds for you).

[samthor]

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 MutationObserver (or something like that) just to automagically upgrade things - in those cases we don't need exports at all.

@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).

[mreinstein]

I've taken another stab at this, simplifying where possible. Here's the current state of things:

• index.js is the source file. It's using es modules and does export default dialogPolyfill;.
• index.js is compiled into a UMD named dialog-polyfill.js this file should work in requirejs, browser globals, and node (commonjs.) I preserved the original filename and path so all the existing examples and tests will continue to work, and should avoid breaking anything for existing users.
• modified package.json with the following changes:
  • "main" field points to dialog-polyfill.js UMD so it works in node
  • "module" field points to index.js so environments that natively understand esm (rollup, webpack v2+) can use the fancy new modern way
  • added prepublishOnly event hook, which is run by npm prior to publishing new versions. This will ensure npm has built the up-to-date dialog-polyfill.js just before publish. If the build step fails, the publish will also fail

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.

[brettz9]

I might suggest making a separate format: "es" distribution (and point module to it instead of source)... Just a bit better practice, imo, to have a convention to find all usable files within dist, especially in case the package ever would rely on a dependency or split the source files (yes, if you use the ".js" extension, split source files could still be imported at run-time, but potentially less desirable for performance than having it all pre-packaged in an "es" format bundle).

[samthor]

Can we leave comments on the PR? #176

[mreinstein]

this issue should be resolved now since the PR was merged. @samthor can we get a new npm release for this please? 🙏

[samthor]

Done https://github.com/GoogleChrome/dialog-polyfill/releases

[lannonbr]

As a continuation of this, if you load in dialog-polyfill with require as such:

const dialogPolyfill = require('dialog-polyfill');

If you bundle this down with webpack, it defaults to resolving the module field before the main field, which results in the actual contents of the module behind dialogPolyfill.default as it is bundling the ESM build rather than the CJS build.

As a fix for this, would it be possible to support the exports field by adding the following to the package.json:

  "exports": {
    ".": {
      "import": "./dist/dialog-polyfill.esm.js",
      "require": "./dist/dialog-polyfill.js"
    }
  },

I did this in a test project and then if you did require('dialog-polyfill') it would include the CJS version in the webpack bundle.

[mreinstein]

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.