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 ESM module using conditional exports #11

Merged
merged 1 commit into from
Sep 13, 2021
Merged

Conversation

il3ven
Copy link
Contributor

@il3ven il3ven commented Sep 12, 2021

@il3ven
Copy link
Contributor Author

il3ven commented Sep 12, 2021

@TimDaub

Copy link
Member

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

In #8 I favored using "option 2" for higher compatibility. I'd like to avoid for users of this library having to set the experimental flag experimental-conditional-exports.

What's the reason that you've favored option 1 over option 2 that promises further compatibility?

@il3ven
Copy link
Contributor Author

il3ven commented Sep 13, 2021

@TimDaub When the issue mentioned 'option 2' I thought it meant 2.x.

I didn't notice the caveat.😅 A reason for implementing option 2.1 was that 2.2 seemed hacky with more than one package.json.

I tested things after implementation and didn't have to add the experimental flag. Otherwise I wouldn't have missed the caveat.

After some digging, I found that the flag has been removed for node 12.x+. (nodejs/node#31001) Currently I have tested on node v12.22.6 and node v14.17.6. Everything works fine without any warnings.

I say we stick with option 2.1. If you think otherwise then I can make changes to this PR.

@TimDaub
Copy link
Member

TimDaub commented Sep 13, 2021

Thanks for digging. Supporting node >= 12 is sufficient.

@TimDaub TimDaub merged commit 68077d4 into attestate:master Sep 13, 2021
@il3ven il3ven deleted the esm branch September 13, 2021 16:22
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.

allow dual use cjs and esm
2 participants