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

Types that are found, but not exported in package.json, should still be usable #52363

Closed
5 tasks done
justinfagnani opened this issue Jan 23, 2023 · 19 comments · May be fixed by samhirtarif/react-audio-visualize#20
Closed
5 tasks done
Labels
External Relates to another program, environment, or user action which we cannot control.

Comments

@justinfagnani
Copy link

Suggestion

With module resolution "Node16" or "NodeNext" I'm running into problems using packages where tsc can find declaration files but won't use them because they're not exported in package.json.

The error I get is:

error TS7016: Could not find a declaration file for module '@web/dev-server'. '/Users/justin/Projects/Web/simple-story-viewer/node_modules/@web/dev-server/index.mjs' implicitly has an 'any' type.
  There are types at '/Users/justin/Projects/Web/simple-story-viewer/node_modules/@web/dev-server/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The '@web/dev-server' library may need to update its package.json or typings.

This means that I can't use the package without copying it's types or patching the package locally with something like patch-package.

I know the package should technically be exporting its types correctly (though I might argue that this is being overly pedantic and that the existence of the these specific files strongly implies that the author intended them to be used), but as a user of TypeScript I'm stuck and tsc seems to have enough information to just laod the types for me so I can at least make progress.

I'd like some sort of option to tell tsc to loosen up about these files. If there's an encapsulation violation or something breaks with the next version of the package I'm using, I'll deal with it.

It's also possible that behavior is a bug. As commented on by @andrewbranch in #46334:

a foolproof way to write a valid project structure and package.json file is just to publish your declaration files in the same place as their partner JS files and literally never write types anywhere in your package.json.

This seems to say that if I'm importing index.mjs and there's an index.d.ts next to it, the types should load. Is that correct?

🔍 Search Terms

  • package export types
  • this result could not be resolved when respecting package.json "exports"

A lot of issues came up in the first search. I could have missed a duplicate.

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

An option to load types not exported in packae.json.

📃 Motivating Example

https://github.com/modernweb-dev/web/blob/master/packages/dev-server/package.json

💻 Use Cases

Packages that aren't ready for TypeScript's strict type loading behavior.

@Andarist
Copy link
Contributor

This seems to say that if I'm importing index.mjs and there's an index.d.ts next to it, the types should load. Is that correct?

Yes, but index.d.ts is not a "sibling" file to index.mjs - that would be index.d.mts.

You can often debug such issues with tsc --traceResolution. If I run this in your repo (after setting up a fixture~ dir) then I can find there this:

...
Matched 'exports' condition 'import'.
Using 'exports' subpath '.' with target './index.mjs'.
File name '/modernweb-dev-web/node_modules/@web/dev-server/index.mjs' has a '.mjs' extension - stripping it.
File '/modernweb-dev-web/node_modules/@web/dev-server/index.mts' does not exist.
File '/modernweb-dev-web/node_modules/@web/dev-server/index.d.mts' does not exist.
...

@justinfagnani
Copy link
Author

So is the error wrong then?

The error is saying that the file just needs to be in exports, but really the issue is that the file has the wrong extension.

@andrewbranch
Copy link
Member

The error is not wrong; it does not say that the file needs to be listed in exports. It says it was unresolvable while respecting exports. If we had never looked at exports, we would have looked at main instead, which has a corresponding typings file.

index.d.ts cannot be used to type index.mjs. The former is obviously a CJS module while the latter is obviously an ES module. The error isn’t saying “hey we found the file you probably meant, but in the interest of being overly pedantic, we’re not going to load it.” It’s saying that we found a typings file, so the author clearly intended to include types, but we have no idea what to load for the ESM entrypoint.

This means that I can't use the package without copying it's types or patching the package locally with something like patch-package.

The hope was that folks would open issues, not just work around it for themselves 😕

@andrewbranch
Copy link
Member

Something that would be nice to have is a way for library authors to indicate that a CJS .d.ts file can undergo some kind of trivial static transformation (in memory, built into tsc) and become an accurate (synthesized) ESM declaration file. That would make it easier for library authors to fix these issues, and then perhaps there could be a flag like you suggest, which would apply that transformation based on filename heuristics.

(In this case, it looks like no syntactic transformation is actually required, but I think in general you need to know whether to set the CJS module exports to the default export of the ES module. For example, if the CJS file module.exports a class or function, its ESM counterpart likely does an export default. On the other hand, if a CJS file module.exports an object literal, the ESM counterpart might export default that object literal, or it might do a named export for each of the object literal keys, or it might do both. It just depends on how the original module was authored and what tool was used to do the dual emit. Consequently, I don’t know that we could automatically pick a correct transform at all—I just wanted to explain why a transform is necessary in general, despite the fact that it would be a no-op on the example you provided. Even in the case that the content of a .d.ts file would be identical if it were changed to be a .d.mts file, we need the infrastructure to be able to treat the .d.ts file as a CJS module when it’s resolved normally but as an ES module when it’s resolved as the stand-in for a missing .d.mts file.)

I’ll continue thinking about this, but I think the version of this option that you’re asking for is too loose. At the same time, so many libraries are misconfigured that everyone would turn it on, and we would forever lose the ability to do something more correct. In the meantime, please do let the library maintainers for @web/dev-server know that they are missing an index.d.mts file.

@andrewbranch andrewbranch added the External Relates to another program, environment, or user action which we cannot control. label Jan 24, 2023
@typescript-bot
Copy link
Collaborator

This issue has been marked as 'External' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@dhruvkelawala
Copy link

dhruvkelawala commented Feb 3, 2023

This seems like a very prominent issues with many packages that I am trying to use in my project with Typescript v5.0-beta and moduleResolution: bundler.
A really common package that many might be using with Vite, also has this issue.

https://github.com/gxmari007/vite-plugin-eslint

@Andarist
Copy link
Contributor

Andarist commented Feb 3, 2023

Here you go, I fixed this for you: gxmari007/vite-plugin-eslint#60

@dhruvkelawala
Copy link

That was fast. Thank you!

@justinfagnani
Copy link
Author

justinfagnani commented Apr 16, 2023

If anyone's still paying attention to this issue... what's the fix when you encounter this error? I haven't been able to upgrade any of my projects to TypeScript 5.0 w/ resolution "bundler" yet because they have too many dependencies that trigger this error.

The types are there, I just want to tell TypeScript to actually use them.

@andrewbranch
Copy link
Member

File an issue or PR against the library. I built https://arethetypeswrong.github.io after closing this issue to help users convince library authors to fix their types. @web/dev-server was fixed in v0.1.36.

@Andarist
Copy link
Contributor

And in the meantime - patch-package is your friend 😉 I know that's not ideal but if you are eager to switch to moduleResolution: node16 then it's a way to do it before your deps catch on.

@lupas
Copy link

lupas commented May 1, 2023

And in the meantime - patch-package is your friend 😉 I know that's not ideal but if you are eager to switch to moduleResolution: node16 then it's a way to do it before your deps catch on.

Thanks @Andarist , just learned about patch-package from you - what a game changer! 🥳

For this case make sure to include changes in package.json by not excluding the file by default like so:

npx patch-package insertPackageName --exclude 'nothing'

@rdunk
Copy link

rdunk commented May 4, 2023

patch-package doesn't support using workspaces as far as I can tell. Does anyone have a solution that works for monorepos (turborepo in my case)?

rxw1 added a commit to rxw1/VuePDF that referenced this issue Jul 7, 2023
Correctly exporting types in package.json. See issue and solution here microsoft/TypeScript#52363 for reference.
@sgpinkus
Copy link

index.d.ts cannot be used to type index.mjs. The former is obviously a CJS module while the latter is obviously an ES module.

@andrewbranch I don't that's obvious to like 90% of Typescript developers and probably a large fraction of Typescript package maintainers. There are some incompatibilities around default exports but I thought that's what the tsconfig esModuleInterop option was for? I think most of us haven't thought further than that. What else is different between the typings for CJS and ESM?

@andrewbranch
Copy link
Member

Oh, this is definitely unintuitive. When I said “obviously” in that message, what I meant was that it’s unambiguous and inflexible to tooling like Node and tsc due to file extensions and package.json "type" lookups. I definitely used the wrong word and wasn’t listening to how it was sounding as it came out, my bad 🤦.

esModuleInterop is a setting that controls how CJS modules that are getting (or have already been) transpiled from ESM syntax refer to each others’ default exports. It’s not possible for it to have any effect on how real ES modules in Node reference CJS modules. So, in a Node project, tsc really needs to know on a file-by-file basis which files are CJS modules (which fall into the esModuleInterop interop rules) and which files are true ES modules (which fall into Node’s interop rules).

The other significant difference between ES modules and CJS modules as far as TypeScript is concerned (that is, things that would make a difference in how declaration files are processed) is the module resolution algorithm is different, so tsc needs to know which algorithm to use (again on a file-by-file basis—actually, on an import-by-import basis, since you can have a ESM dyanmic import in a CJS file or a CJS createRequire in an ESM file) to resolve imported paths.

I have some new module docs in progress at #52593 and an appendix on ESM/CJS interop is already done if you want to dive into more details: https://gist.github.com/andrewbranch/79f872a8b9f0507c9c5f2641cfb3efa6#file-04_01_esm-cjs-interop-md

Also, for package authors, I recently invested a lot into the docs for https://arethetypeswrong.github.io, so if an author tries to use a .d.ts file to type an .mjs file, they’ll be directed to this document which I think explains the specific problem in sufficient detail.

cabello added a commit to cabello/add-to-calendar-button-react that referenced this issue Jul 31, 2023
Fix:

```
Could not find a declaration file for module 'add-to-calendar-button-react'. 'node_modules/add-to-calendar-button-react/dist/atcb.js' implicitly has an 'any' type.
  There are types at 'node_modules/add-to-calendar-button-react/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'add-to-calendar-button-react' library may need to update its package.json or typings.

import { AddToCalendarButton } from "add-to-calendar-button-react"
```

I checked the related issues from this main issue and the common way to fix it, is by exporting the types as well: microsoft/TypeScript#52363
@cmdruid
Copy link

cmdruid commented Jul 31, 2023

What a complete mess. I am so tired of typescript breaking things and then blaming package authors for "doing it wrong."

If new version typescript can't figure out how to apply types to .cjs and .mjs for 95% of packages then that is the fault of typescript. The typing should be agnostic by default, or the 5% edge cases should provide clearer types for their obscure use of imports.

Having to use patch libraries or copy/paste type declarations is a joke. Sticking to 4.9.4 and "moduleResolution": "node" until this gets sorted out.

@andrewbranch
Copy link
Member

If new version typescript can't figure out how to apply types to .cjs and .mjs for 95% of packages then that is the fault of typescript.

Totally agree. If 95% of packages had this problem, that would force us to consider a different design. Luckily, I have the numbers, and it’s 11% of types-shipping packages (area of red + green here; y-axis is percentage expressed as a ratio; sampled from thousands of the most downloaded / depended upon packages):

MicrosoftTeams-image

I don’t want anyone to get the impression that TS is blaming package authors. I explained once:

When I say some package is “wrong” it’s always an observation about what works and doesn’t work with the tools that exist today, not a value judgment about what was published in the past. I recognize why that can rub some the wrong way, but arethetypesincompatiblewithtypescripttoday.github.io didn't have the same ring to it 😅

I’m not very interested in assigning blame, but if I have to, we dropped the ball on communicating how this was going to work early enough and clearly enough. We’re all on the same side though; everyone wants stuff to work perfectly for a wide variety of users in a huge matrix of different scenarios. I have thought a lot about how we could do things differently today to make that easier, and better tools + docs that explain how things work today was the top priority. Unfortunately, there’s no magic wand we can wave over the TypeScript codebase and make all the existing typed npm packages work perfectly. We just have to do better about helping library authors make valid ones in the future, and help chip away at the ones that already have problems.

@cmdruid
Copy link

cmdruid commented Jul 31, 2023

If 95% of packages had this problem, that would force us to consider a different design. Luckily, I have the numbers, and it’s 11% of types-shipping packages

Isn't this issue in regards to packages that provide mjs and cjs exports with a single declaration of types, not just any package with types? I feel like you are conflating this with a bunch of packages that don't even relate to what is going on here. In any case, breaking 11% of the top packages is still a huge figure.

I realize now that I am forced to fix this issue at metaphorical gun-point, because nextjs (and I guess vue) templates are shipping with this new version by default. So I have to do this or else all typing with my packages break with this new update. Great.

For anyone else reading, I resolved the issue by replacing my exports with this new boilerplate:

"exports": {
  ".": {
    "import": {
      "types": "./dist/types/index.d.ts",
      "default": "./dist/module.mjs"
    },
    "require": {
      "types": "./dist/types/index.d.ts",
      "default": "./dist/main.cjs"
    }
  }
},
"types": "./dist/types/index.d.ts",
"main": "./dist/main.cjs",

I still don't understand the purpose of any of these breaking changes, why the exports format has to change yet again, or why I'm forced to make this change when it was working perfectly fine before.

@andrewbranch
Copy link
Member

That exports configuration doesn’t fix the issue; it’s identical to one where "types", "import", and "require" are siblings. The key issue is that a single declaration file cannot describe two modules of different formats, even if the syntax within is the same. The file extension itself is an important piece of metadata that affects type checking. There’s no way to fix the package.json snippet there without having two separate index.d.cts and index.d.mts files.

I still don't understand the purpose of any of these breaking changes

Here are some resources.

@microsoft microsoft locked as resolved and limited conversation to collaborators Jul 31, 2023
asquithea added a commit to asquithea/react-audio-visualize that referenced this issue Dec 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
External Relates to another program, environment, or user action which we cannot control.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants