search

manuelbieh / geolib

Zero dependency library to provide some basic geo functions
MIT License
4.23k stars 342 forks source link

Add `types` condition to the `require` condition #299

Closed Andarist closed 1 year ago

Andarist commented 1 year ago

Right now, the current setup works in TypeScript but it's considered a bug and it should not be relied upon, see the thread and the comment here. For that reason, I would like to fix all popular packages that misconfigured their exports this way so the bug can be fixed in TypeScript.

You can check the reported error here

manuelbieh commented 1 year ago

Wow, thanks!

Andarist commented 1 year ago

@manuelbieh I meant to add some more comments but had to go away from the computer before I could do that 😅 so here we go:

I think that the current setup is still not quite OK:

  1. package.json#module points to es directory and that contains CJS files. This should point ESM files - but also keep in mind that package.json#module isn't "standard". It's a field only understood by bundlers - it's still pretty popular and its semantics are somewhat well-defined.
  2. you have require+default conditions here. Both could be merged into just default~ (and that could allow for "collapsing" the whole "." entry to just ".": "./es/index.js"). Right now the default contains a reference to an UMD bundle but that seems to be pretty redundant to me.

All in all, I just wanted to fix a specific problem here - without introducing too many changes at once but this setup, imho, could use some further work.

Andarist commented 1 year ago

@manuelbieh would there be a chance to cut a new release with this small fix?

manuelbieh commented 1 year ago

Hey. I'll look into that. It should have actually been released via CI. Don't know why this didn't happen. I will check 😉

manuelbieh commented 1 year ago

:tada: This PR is included in version 3.3.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket: