Skip to content
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

Add Type: Module to Package.Json #47

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Add Type: Module to Package.Json #47

merged 3 commits into from
Nov 19, 2024

Conversation

ingalls
Copy link
Contributor

@ingalls ingalls commented Nov 14, 2024

👋 Hey @mapsam,

Thanks a ton for the TS migration, it's wonderful to replace my shims! The current version (2.0.0) is causing some issues with my deploy tooling as the TS package uses Module exports syntax without declaring itself a module.

I've added the type: module in the package.json required to allow use of exports instead of module.exports

@mapsam
Copy link
Contributor

mapsam commented Nov 15, 2024

Woops thanks for the ping @ingalls, sorry about the error there! I'm no longer able to merge these PRs, but maybe @greenlaw or @AlexanderBelokon can take a look.

@ingalls
Copy link
Contributor Author

ingalls commented Nov 15, 2024

I saw that after I cut this PR!! Congrats on the new gig!

@greenlaw
Copy link
Contributor

@ingalls I think this will break CJS imports if merged, is that your understanding?

We just went through this on another repo and ended up using a dual-packaging approach. Can you take a look here and see if the same approach would work for you here? mapbox/sphericalmercator#47

@ingalls
Copy link
Contributor Author

ingalls commented Nov 15, 2024

@greenlaw Yes, you are correct, my head is 100% into ESM so I haven't dealt with compat with CJS in a bit but your approach in SM seems like the way to go to retain CJS support.

I'd be happy to cut a PR following your approach with SM tomorrow if that would be useful.

@mapsam
Copy link
Contributor

mapsam commented Nov 15, 2024

Thanks y'all! 🙌

@greenlaw
Copy link
Contributor

@ingalls That would be great - thanks

@ingalls
Copy link
Contributor Author

ingalls commented Nov 15, 2024

@greenlaw I've updated the PR with the same dual ESM/CJS approach you took in the SM library. Let me know if this looks good to you! Thanks all!

@greenlaw
Copy link
Contributor

@ingalls Looks good, just two minor things:

  • I think we can keep the prepublishOnly script
  • Could you bump the version to 2.0.1 and npm i to update package-lock.json again?

Happy to approve/merge once that's done.

@ingalls
Copy link
Contributor Author

ingalls commented Nov 18, 2024

@greenlaw done.

Copy link
Contributor

@greenlaw greenlaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ingalls !

@greenlaw greenlaw merged commit 72fb86c into mapbox:main Nov 19, 2024
@greenlaw
Copy link
Contributor

v2.0.1 has been published 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants