support esm, cjs modules #164 and remove bower support

samthor commented 5 years ago

is there a way we can do this without duplicating the code?

mreinstein commented 5 years ago

what code duplication are you referring to?

samthor commented 5 years ago

.cjs.js, .esm.js, are all basically clones of the main js file. Can we do this as a build step?

mreinstein commented 5 years ago

So more like a umd bundle? We could do this, though the downside is environments that support es modules natively won't actually have an es module. We could do 2 builds maybe? umd and esm ?

samthor commented 5 years ago

This is looking good. Do we have to leave dialog-polyfill.js as part of the source, or could we instead only build it when we're deploying to NPM?

Mostly again I worry about duplication. At least we'd like to maybe put a comment at the top of dialog-polyfill.js saying that this is not canonical, edit index.js for actual changes.

mreinstein commented 5 years ago

Do we have to leave dialog-polyfill.js as part of the source

I mostly left dialog-polyfill.js as-is for 3 reasons:

it would be the least change for anyone already using this project (it would still work as a script global)
all the tests and demo scripts except that file to be a script global, so those continue to work as-is
I'm not sure if people are doing npm install than pointing at node_modules/dialog-polyfill/dialog-polyfill.js in their html files (which is bananas in my opinion but some people do it)

That said none of these 3 reasons are critical for keeping things as-is. Could change it and simply bump the major version number of the module to (0.5.0 maybe?) and set it up however is desired. :)

mreinstein commented 5 years ago

another thing we could do is restructure the project like this:

src/index.js <- intuitively seems like the entry point
dist/dialog-polyfill.js <- the built version used in browser and node environments.

dist/ seems to be a common convention for "these are built files"

mreinstein commented 5 years ago

@samthor if you have follow up thoughts happy to make changes to move this along. 👍

samthor commented 5 years ago

I'm sorry for dragging my heels on this.

My only major comment is that I'd either like:

to note in the README somewhere that dialog-polyfill.js is purely generated as part of a build process
to not include it at all—since users of this are now getting it from NPM, the artefact will appear there.

googlebot commented 5 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot commented 5 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mreinstein commented 5 years ago

I've pushed a commit which I believe resolves all of the above feedback. I've also signed the cla (though I'm not sure why it's still showing that I haven't. Maybe it just takes a moment to update.)

I think it might be valuable to outline all of the supported install vectors:

directly include the script as a global in an html document

<script src="node_modules/dialog-polyfill/dialog-polyfill.js"></script>

from a script module

<script type="module">
import dialogPolyfill from './node_modules/dialog-polyfill/dialog-polyfill.esm.js';
// ...
</script>

a nodejs script, or a script bundled via commonjs (browserify)
```
const dialogPolyfill = require('dialog-polyfill');
```
a nodejs script using the esm module (node -r esm script.js) or bundled via esm (rollup, webpack)
```
import dialogPolyfill from 'dialog-polyfill';
```

deno (which is awesome by the way https://deno.land )

import dialogPolyfill from `https://raw.githubusercontent.com/GoogleChrome/dialog-polyfill/master/dialog-polyfill.esm.js`;

The current state of this PR should support all of these (and I'm assuming we want to.) Open for suggestions, I just wanted to make these use cases more up-front and explicit.

googlebot commented 5 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot commented 5 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

samthor commented 5 years ago

There's now three copies of the same code again and no clear indication in the README which one is the source of truth.

Again, I'd much rather just have one file here and only generate the needed output as an artefact of we upload to NPM.

(edit) I do appreciate y'all sticking with me on this, I do think it's a better solution, but it needs to be clear.

mreinstein commented 5 years ago

I'd much rather just have one file here and only generate the needed output as an artefact of we upload to NPM.

I understand the utility of having one source file so it's clear what the "source of truth" is. The problem is, there are some install vectors that have nothing to do with npm. script global inclusion for example. People that embed dialog-polyfill in their website without using any package manager are likely to just clone your repo, or just copy the file directly off of the github page. In that case no npm build step runs, and the source of truth file can't be included directly.

See my above comment that attempts to list all known install vectors. Are some of these ones we don't care about?

@samthor again, I'm happy to make changes but the problem is there are a lot of people weighing in with feedback, and some of it is conflicting. If you give me explicit changes that you want, I'll make them.

mreinstein commented 5 years ago

some possible solutions to the multiple source file copy concern:

dialog-polyfill.esm.js can be eliminated. It is a byte-for-byte copy of index.js
add a CONTRIBUTING section to the readme saying something along the lines of "please modify index.js, it is the source of truth that other files are generated from"
add a commented banner at the top of the generated files saying something along the lines of // this file is generated from index.js, please modify that instead and rebuild with "npm run build"

samthor commented 5 years ago

I'm happy with the first two changes. The last is redundant as folks will hopefully see the README.

On Wed, 27 Feb 2019 at 13:59, Mike Reinstein notifications@github.com wrote:

some possible solutions to the multiple source file copy concern:

dialog-polyfill.esm.js can be eliminated. It is a byte-for-byte copy of index.js

add a CONTRIBUTING section to the readme saying something along the lines of "please modify index.js, it is the source of truth that other files are generated from"

add a commented banner at the top of the generated files saying something along the lines of // this file is generated from index.js, please modify that instead and rebuild with "npm run build"

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/pull/176#issuecomment-467704377, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHRkHq6yLr9gGdn6RoXBE56TFPi72bAks5vRfSOgaJpZM4agcOP .

mreinstein commented 5 years ago

we could also create a dist/ directory and put all the built files in that. I feel like dist/ is pretty well understood to mean "these files are the result of a build process", and we could put something in the readme saying that explicitly

samthor commented 5 years ago

Yes, that's even clearer. I'd be very happy with that. 👍

On Wed, 27 Feb 2019 at 14:12, Mike Reinstein notifications@github.com wrote:

we could also create a dist/ directory and put all the built files in that. I feel like dist/ is pretty well understood to mean "these files are the result of a build process", and we could put something in the readme saying that explicitly

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

brettz9 commented 5 years ago

As per above, I think the dist solution is best even if it ends up being a byte-for-byte copy in the case of the ESM copy because again, if the source ever splits into multiple files or comes to rely on third party dependencies, consumers of the ESM distribution won't have a breaking change to change paths to point to a separate bundled distribution script. It can be already anticipated and available there now without future breaking changes.

samthor commented 5 years ago

Yeah, if we have a dist folder I'm happy to have the duplication if you think it's important. It's a clear artefact at that point.

On Wed, 27 Feb 2019 at 14:17, Brett Zamir notifications@github.com wrote:

As per above, I think the dist solution is best even if it ends up being a byte-for-byte copy in the case of the ESM copy because again, if the source ever splits into multiple files or comes to rely on third party dependencies, consumers of the ESM distribution won't have a breaking change to change paths to point to a bundled distribution script. It is already anticipated and there.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/pull/176#issuecomment-467707787, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHRkMDrHcziho06g8p3fwzKzVN2yUUwks5vRfjWgaJpZM4agcOP .

mreinstein commented 5 years ago

do we want to build the contents of dist/ only before publishing to npm, or would we also like to do it just before committing? I could add a git pre-commit hook if this is desirable.

samthor commented 5 years ago

Before publishing. I don't want dev work to generate artefacts in PRs. Unless this is breaking with traditional precent.

On Wed, 27 Feb 2019 at 14:23, Mike Reinstein notifications@github.com wrote:

do we want to build the contents of dist/ only before publishing to npm, or would we also like to do it just before committing? I could add a git pre-commit hook if this is desirable.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mreinstein commented 5 years ago

it's up to you, different projects handle it differently. Though I will say the one nice thing about doing it on pre-commit; if the build step fails, the commit also fails.

brettz9 commented 5 years ago

I personally don't like pre-commit hooks in case I am working on a fork which is partially complete, but I'm not working on the source.

I do, however, like including the dist files in source at least for publish time, as I think it can be a good guard against inadvertent changes which affect the build--e.g., if a change shouldn't affect the build but it does.

But feel free to ignore--just my own prefs...

mreinstein commented 5 years ago

there are definitely pros and cons to the pre-commit hook(s). I don't feel that strongly about it either way.

btw when we get these details sorted let me know and I'll re-submit a clean PR.

mreinstein commented 5 years ago

have been going back and forth between dist/dialog-polyfill.js vs dist/dialog-polyfill.umd.js.

Also, dialog-polyfill.css vs dist/dialog-polyfill.css

thoughts?

samthor commented 5 years ago

Since this is such a big change (and will probably be a 0.x bump) let's copy (?) the CSS as part of build, to dist (giving us more options in future). It matches the model of mirroring the module code.

And I don't see the value in ".umd", it's confusing for folks using it the old-style way.

On Wed, 27 Feb 2019 at 14:37, Mike Reinstein notifications@github.com wrote:

have been going back and forth between dist/dialog-polyfill.js vs dist/dialog-polyfill.umd.js.

Also, dialog-polyfill.css vs dist/dialog-polyfill.css

thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/pull/176#issuecomment-467711310, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHRkJD8fjel5uv77YcZ3yzqx3ecf2g2ks5vRf2VgaJpZM4agcOP .

mreinstein commented 5 years ago

I believe this should resolve all the open items. Let me know if I've missed anything.

samthor commented 5 years ago

The build:esm and build:umd steps are the wrong way around. Otherwise LGTM

GoogleChrome / dialog-polyfill

support esm, cjs modules #164 and remove bower support #176