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: Add types to exports field in package.json to resolve type errors #368

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

doinki
Copy link
Contributor

@doinki doinki commented Aug 28, 2024

closes: #367

CleanShot 2024-08-28 at 16 37 34

@zpao
Copy link
Owner

zpao commented Aug 28, 2024

Bah I thought this was working fine :( Thanks for sending a fix!

Not at a computer to test something but I'm curious if this would have been fine if the .d.mts was moved into lib/esm...

@doinki
Copy link
Contributor Author

doinki commented Aug 29, 2024

@zpao I tested it by moving only the lib/index.d.mts file to the lib/esm/ directory without adding the type to the exports field, but I still get a type error.

When I renamed lib/esm/index.d.mts to lib/esm/index.d.ts, the type error no longer occurred.

@zpao
Copy link
Owner

zpao commented Aug 29, 2024

Ok back to a computer. Thanks for testing! I was wondering if there was some default behavior we could piggyback on.

That said, I'm struggling to reproduce the issue with the published package :( I think what you have is technically better so we should still do it but would like to pinpoint the precise conditions that cause the problem. I created a theoretical minimal test case - https://gist.github.com/zpao/af370e6d6a08a07740b8c80b0b76ada2. I used pnpm since you were, but couldn't repro with npm either. I tried node 18, 20, 22, but no luck. Is there perhaps a specific version of node or typescript that does make it happen?

@doinki
Copy link
Contributor Author

doinki commented Aug 29, 2024

@zpao
Oh, sorry. The problem is reproduced when module, moduleResolution, and strict are set as follows in tsconfig.json.

{
  "compilerOptions": {
    ...,
    "module": "ESNext",
    "moduleResolution": "Bundler",
    "strict": true
  }
}

And I tested it by creating a project with next.js.

pnpm create next-app@latest

@zpao
Copy link
Owner

zpao commented Aug 29, 2024

Ah, I should have just set up Next.js and tried that. Looks like "moduleResolution": "bundler" also triggers the issue. Either option alone or together are a problem. fun.

Ok, I think it might be safe to remove the main and module fields and just rely on exports but I don't want to mess with that in a patch release, so what you have lgtm.

Thanks for sticking with me here!

Copy link
Owner

@zpao zpao left a comment

Choose a reason for hiding this comment

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

Thank you!

@zpao zpao merged commit 5837f8f into zpao:trunk Aug 29, 2024
8 checks passed
@zpao zpao added this to the 4.0.1 milestone Aug 29, 2024
zpao added a commit that referenced this pull request Aug 29, 2024
I should really stop using make but it's muscle memory now
zpao pushed a commit that referenced this pull request Sep 1, 2024
#368)

closes #367

(cherry picked from commit 5837f8f)
(updated to correct file reference)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When I install version 4, I encounter a type error.
2 participants