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 export to package.json for TypeScript resolution #43

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

JeffBeltran
Copy link
Contributor

This commit adds an explicit export for the TypeScript type definitions in the package.json file. It ensures that the types are correctly resolved by TypeScript in environments that adhere to the strict module resolution rules defined by the package's exports field. By including the "types" condition within the "." export, TypeScript tooling can now properly locate the index.d.ts file when the sanity-image package is imported.

This commit adds an explicit export for the TypeScript type definitions in the package.json file. It ensures that the types are correctly resolved by TypeScript in environments that adhere to the strict module resolution rules defined by the package's exports field. By including the "types" condition within the "." export, TypeScript tooling can now properly locate the index.d.ts file when the sanity-image package is imported.
@coreyward
Copy link
Owner

Thanks for this, Jeff. For my own edification, are you able to find any documentation on this behavior? I ask because as I understand it, the parent package.json still applies, and that's where the types are defined. This works fine for me in other apps (Next, Remix, Gatsby) using TS 4.x and 5.x in VSCode. For example, here's a component where I use SanityImage in a Next 13 app where types are resolved and comments are coming through for props:

Screenshot 2023-11-27 at 9 38 54 AM

Which is all to say, I'd like to know what has changed or what tooling I'm not considering when I build this out.

@JeffBeltran
Copy link
Contributor Author

I ran into an issue while upgrading a Remix project. It seemed to originate from a modification in the tsconfig.json, where the "moduleResolution" setting changed from "Node" to "Bundler". See this Remix pull request

With "moduleResolution": "Bundler", TypeScript defers module resolution to the bundler, differing from the traditional Node.js approach. I Think this led to TypeScript overlooking the type definitions, expecting the bundler to leverage the exports field in package.json.

To double-check that this wasn't just a Remix issue, I spun up a brand-new Next.js app with npx create-next-app@latest. I ran into the same error for sanity-image: Could not find a declaration file for module 'sanity-image'. Sure enough, Next.js's tsconfig.json also uses "moduleResolution": "Bundler" by default.

From what I gather, adding the type definitions to the exports field in sanity-image's package.json makes it more compatible to ES modules. Admittedly, this isn't my area of expertise, but I did test this locally and it resolved the error.

@coreyward
Copy link
Owner

Interesting. I read through the TS documentation on the bundler option and while it makes some sense, I'm a little unclear as to why the change would affect how this is working.

This all said, this example seems to suggest that types should be under exports like your changes have it, so that might be the way to go. The only thing left to do is to test dropping types at the top-level of the package.json file both on a bundler app and on an app using node16 module resolution. If you're able to do that and report back I'd appreciate it—otherwise I can try to find time next week to do so.

@JeffBeltran
Copy link
Contributor Author

yeah my understanding is you would have both of them? not really clear either... but yeah i could poke around and see if we can't find the correct way to do this

@JamesSingleton
Copy link

I just ran into this issue as well in a Next.js project. At first I was confused because in one project it doesn't give a type error but in my current one it does. I looked at my tsconfig.json and noticed my old one has "moduleResolution": "node" and the new one had "moduleResolution": "bundler". It looked like create-next-app defaults TypeScript projects with "moduleResolution": "bundler" now. https://github.com/vercel/next.js/blob/canary/packages/create-next-app/templates/app/ts/tsconfig.json#L11

@coreyward coreyward merged commit 7a3286d into coreyward:main Dec 13, 2023
@coreyward
Copy link
Owner

I've gone ahead and released this in v0.1.7 and confirmed that it fixes issues when using "moduleResolution": "bundler" in tsconfig.json.

@JeffBeltran Thank you again for the contribution! If you do get around to checking if dropping types from the top-level of package.json I'd love to know what you find. Otherwise I'll just leave the declaration duplicated until I muck with the exports again in the future.

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.

3 participants