GoogleChrome / dialog-polyfill

Polyfill for the HTML dialog element
BSD 3-Clause "New" or "Revised" License
2.45k stars 245 forks source link

Fix types for ESM #210

Closed qurben closed 3 years ago

qurben commented 3 years ago

Fixes GoogleChrome/dialog-polyfill#204

Should not have an impact on the cjs types.

google-cla[bot] commented 3 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

googlebot commented 3 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

qurben commented 3 years ago

@googlebot I signed it!

googlebot commented 3 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla[bot] commented 3 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

samthor commented 3 years ago

So I confess I haven't been paying that much attention to the other issue. I appreciate that there is a problem, but I am confused. The dist esm file has this line in it:

export default dialogPolyfill;

That's literally the default export. It seems like this PR just adds another variable called dialogPolyfill of the core type. But we don't export that; we export a default.

qurben commented 3 years ago

I missed a line when applying my changes, sorry!

The created variable should be exported as the default. This tells Typescript that a value is exported an not a type.

samthor commented 3 years ago

Thanks. I'm learning about TS types vs variables too from this. I'll release an update soon.