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' entrypoint #104

Closed
FredKSchott opened this issue Feb 13, 2020 · 2 comments · Fixed by #105
Closed

Add ESM 'module' entrypoint #104

FredKSchott opened this issue Feb 13, 2020 · 2 comments · Fixed by #105

Comments

@FredKSchott
Copy link
Contributor

Pulled off from: https://www.pika.dev/npm/snowpack/discuss/71

Hey @bcherny, thanks for the great open-source library. A Snowpack user was having some trouble building your package, and I thought I'd loop you in. Since Snowpack is powered by Rollup, there's a good chance that all Rollup users are having trouble with undux due to circular references in your "main" Common.js (CJS) entrypoint.

Circular dependency: node_modules/undux/dist/src/index.js -> node_modules/undux/dist/src/react/index.js -> node_modules/undux/dist/src/react/connect.js -> /Users/crabasa/Code/fibonacci/node_modules/undux/dist/src/index.js?commonjs-proxy -> node_modules/undux/dist/src/index.js
Circular dependency: node_modules/undux/dist/src/index.js -> node_modules/undux/dist/src/react/index.js -> node_modules/undux/dist/src/react/createConnectedStore.js -> node_modules/undux/dist/src/index.js
Circular dependency: node_modules/undux/dist/src/index.js -> node_modules/undux/dist/src/react/index.js -> node_modules/undux/dist/src/react/createConnectedStoreAs.js -> node_modules/undux/dist/src/index.js
Error: '__moduleExports' is not exported by node_modules/undux/dist/src/index.js
    at error (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:5400:30)
    at Module.error (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:9820:16)
    at handleMissingExport (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:9721:28)
    at Module.traceVariable (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:10159:24)
    at ModuleScope.findVariable (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:8766:39)
    at Identifier$1.bind (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:4403:40)
    at ExportDefaultDeclaration.bind (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:3152:23)
    at Program$1.bind (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:3148:31)
    at Module.bindReferences (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:9792:18)
    at Graph.link (/Users/crabasa/Code/fibonacci/node_modules/rollup/dist/shared/node-entry.js:13425:20)

Since bundlers are better at code-splitting/tree-shaking ESM than CJS, and since your only consumers today are using this library through bundlers, have you thought at all about adding a "module" entrypoint to your package.json so that your users get better-optimized bundles? "module" is the entrypoint that 90,000+ packages use on npm to expose their ESM entrypoint (we track this on pika.dev).

I saw that you have a "main:esnext" build, but since TypeScript's esnext target can include experimental JavaScript, this may not be the best choice for a library. target: es2020 would be better only so that bundlers would be guaranteed to understand the code and not break on any experimental (not yet ratified as ES20XX) bits.

There are a few ways to go about this. How would you feel about a PR that changed the tsconfig.esnext.json "target" property from "esnext" to "es2020', and then updated your package.json to add a "module" property pointing to that build? That way this library would be less likely to break Snowpack & Rollup users, and all users bundlers would benefit from a faster ESM library in production.

@FredKSchott FredKSchott changed the title Add ESM 'module' build Add ESM 'module' entrypoint Feb 13, 2020
@bcherny
Copy link
Owner

bcherny commented Feb 13, 2020

Sounds reasonable. Happy to stamp a PR that does that.

@FredKSchott
Copy link
Contributor Author

Awesome, created a PR at #105 👍

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 a pull request may close this issue.

2 participants