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 build and publish config for CommonJS #8

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

desko27
Copy link
Owner

@desko27 desko27 commented Aug 17, 2024

Fixes #6

@desko27 desko27 mentioned this pull request Aug 17, 2024
Copy link

@mariusGundersen mariusGundersen left a comment

Choose a reason for hiding this comment

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

According to this article and others I have read you need to remove the "type": "module" line for it to work. I tried with those changes locally at that made it work for me.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@desko27
Copy link
Owner Author

desko27 commented Aug 19, 2024

Hi @mariusGundersen, thanks a lot for your involvement!

I see, having the main.cjs file as main is the "CJS first" approach. I was trying an "ESM first" approach, but I can see that's not gonna work. I didn't realize even Vite.js recommends CJS first.

But they also recommend keeping "type": "module" and I'm actually getting build errors if I remove it. I've published react-call@next again with your suggested approach but keeping type module. Could you check if that works for you? Thanks for your patience.

@mariusGundersen
Copy link

Yes, now it works! It seems like the problem was "default": "...", it works much better with "import": "...", probably because the default is controlled by "type": "module".

@mariusGundersen
Copy link

Thank you for taking the time to fix this for me 🥳

@desko27
Copy link
Owner Author

desko27 commented Aug 19, 2024

Glad to hear that! I've also been reading a bit more about it just to make sure. According to Vite.js CJS troubleshooting what we're doing matches the "Configure ESM as default, opt-in to CJS if needed" approach, which I think is great to remain in the modern direction!

I'll check everything is still working properly for ESM consumers and we'll get this published on v1.2.0.

@mariusGundersen thanks for contributing!

@desko27 desko27 marked this pull request as ready for review August 19, 2024 11:50
@desko27 desko27 merged commit 40d3570 into main Aug 19, 2024
3 checks passed
@desko27 desko27 deleted the feature/add-build-and-publish-config-for-commonjs branch August 19, 2024 12:54
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.

Publish commonJS modules
2 participants