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

Rework how cjs and esm are supported #8

Closed
wants to merge 5 commits into from

Conversation

SirHall
Copy link

@SirHall SirHall commented Jul 10, 2024

The recent changes where the esm file imported the built cjs files is causing havoc with our build tooling (primarily around it incorrectly resolving default imports), generally it is more typical to output ESM and CJS build files separately which work a lot better with rollup/esbuild (we're using vite) and should prevent anyone else having issues with this either.

Thanks.

@satya164
Copy link
Owner

satya164 commented Jul 10, 2024

Thanks for the PR.

This approach was taken from Node.js docs https://nodejs.org/api/packages.html#approach-1-use-an-es-module-wrapper

I think the approach in the PR won't work on Node.js:

  • NodeJS requires explicit file extensions for ESM, and we cannot use explicit file extensions or .native.ts won't work
  • It's also not correct as the commonjs build has .js extension and not .cjs - hence it'll be treated as ES module and not CommonJS due to type: 'module' in the package.json.

I have added some tests for modules which show this behavior.

We need to figure out if it's possible to make the current approach work. I'm going to spend some more time to investigate it. Any pointers are welcome.

@satya164
Copy link
Owner

satya164 commented Jul 10, 2024

I just tried importing the package in a fresh vite app and it seems to work properly. Can you share a repro and more details about what issue you are facing?

@satya164
Copy link
Owner

Closing due to no response.

@satya164 satya164 closed this Jul 14, 2024
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.

2 participants