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

fix: allow exports type import #169

Merged
merged 2 commits into from
Jul 9, 2021
Merged

fix: allow exports type import #169

merged 2 commits into from
Jul 9, 2021

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Jul 4, 2021

close #168

@dai-shi
Copy link
Member Author

dai-shi commented Jul 4, 2021

@lxsmnsyc Does it look OK? Can you confirm?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 4, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 88ac94f:

Sandbox Source
React Configuration
React Typescript Configuration

@lxsmnsyc
Copy link
Contributor

lxsmnsyc commented Jul 5, 2021

I'll try and test it out locally

@lxsmnsyc
Copy link
Contributor

lxsmnsyc commented Jul 6, 2021

Sorry for the late reply.

It seems that it's not just the Symbols that are supposed to be the shared state between the ESM and CJS builds, here are others:

https://github.com/pmndrs/valtio/blob/master/src/vanilla.ts#L10

https://github.com/pmndrs/valtio/blob/master/src/vanilla.ts#L29-L36

https://github.com/pmndrs/valtio/blob/master/src/react.ts#L39-L48

At this point, I suggest to drop the ESM build and follow the 1st approach: https://nodejs.org/api/packages.html#packages_approach_1_use_an_es_module_wrapper

the only downside here is that we have to manually redefine all exports if the exports changes.

About the package.json, everything seems fine.

@dai-shi
Copy link
Member Author

dai-shi commented Jul 6, 2021

Oh, good catch. Yeah, they are module states.

I'm skeptical about the approach #1.

// ./node_modules/pkg/wrapper.mjs
import cjsModule from './index.cjs';
export const name = cjsModule.name;

This may work in node, but not sure all bundlers support this. importing .cjs seems non-traditional.

At this point, I guess we should accept dual package hazard, and try to educate community and library authors.

@lxsmnsyc
Copy link
Contributor

lxsmnsyc commented Jul 6, 2021

This may work in node, but not sure all bundlers support this.

In ESBuild and Rollup, this would work for both CJS and ESM formats.

importing .cjs seems non-traditional.

The ESM -> CJS bridging is an anti-pattern. An example of this would be React. However, it lead React to not have tree-shaking

At this point, I guess we should accept dual package hazard, and try to educate community and library authors.

I agree with this statement.

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.

Unsupported export map resolution
2 participants