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

Export both CJS and ESM for node #37

Closed
wants to merge 3 commits into from

Conversation

monsterbitar
Copy link

See #30 (comment) for details.

add ESM compatible node version
"types": "index.d.ts",
"browser": "browser.js",
Copy link

Choose a reason for hiding this comment

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

Please left the browser field to keep backwards compatibility, it's not standard but some old tools could still be using it.

Copy link
Author

Choose a reason for hiding this comment

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

I'm ok with this if that is what the package maintainer wants.

If they do, this PR has "allow edits by maintainers" enabled and should be an easy fix.

@JoCat
Copy link

JoCat commented Jun 16, 2023

LGTM
I did the same thing earlier in my version of the package and as far as I know everything works correctly

@monsterbitar
Copy link
Author

For reference, I understand why this is a difficult thing to get right - after forking and maintaining this change I've had numerous situation where this doesn't work.

I don't expect this to get merged, and have since opted to ditch CJS support entirely in my own local instance.

Feel free to re-open or use what makes sense if focus is turned back to this later.

@jeswr
Copy link

jeswr commented Aug 7, 2023

For reference, I understand why this is a difficult thing to get right - after forking and maintaining this change I've had numerous situation where this doesn't work.

@monsterbitar is this still the case if you keep original browser entry as suggested in #37 (comment).

If not would it be possible for this work to be released with that changed?

@monsterbitar
Copy link
Author

For reference, I understand why this is a difficult thing to get right - after forking and maintaining this change I've had numerous situation where this doesn't work.

@monsterbitar is this still the case if you keep original browser entry as suggested in #37 (comment).

If not would it be possible for this work to be released with that changed?

yes, the issue is much deeper and there's just too many different variations and packaging setups that are close-but-not-quite-the-same across the board.

Since browsers have supported ESM for quite some time now though, I'm simply looking forward to when we drop CJS altogether and remove the additional complexity.

@piranna
Copy link

piranna commented Aug 7, 2023

I have migrated to unws package, that's similar to this one but with full esm support.

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.

4 participants