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

Typescript/export/module/package.json B.S. #57

Closed
tomlarkworthy opened this issue Aug 25, 2023 · 14 comments
Closed

Typescript/export/module/package.json B.S. #57

tomlarkworthy opened this issue Aug 25, 2023 · 14 comments

Comments

@tomlarkworthy
Copy link

image
@tomlarkworthy
Copy link
Author

smh microsoft/TypeScript#52363

@Cosmic-Ryver
Copy link

I ran into this problem today as well. The exports field in package.json just needs a "types" entry.

Diff that solves the problem:

diff --git a/node_modules/aws4fetch/package.json b/node_modules/aws4fetch/package.json
index 4ea1e45..95842c7 100644
--- a/node_modules/aws4fetch/package.json
+++ b/node_modules/aws4fetch/package.json
@@ -14,7 +14,8 @@
       "worker": "./dist/aws4fetch.esm.js",
       "browser": "./dist/aws4fetch.umd.js",
       "require": "./dist/aws4fetch.cjs.js",
-      "default": "./dist/aws4fetch.umd.js"
+      "default": "./dist/aws4fetch.umd.js",
+      "types": "./dist/main.d.ts"
     }
   },
   "types": "dist/main.d.ts",

This was partially generated by patch-package.

@tomlarkworthy
Copy link
Author

tomlarkworthy commented Aug 25, 2023

I don't think so, I think every export needs one, it also says the types should come first, and I should not have deleted the top level one as that's a fallback

https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

@mhart
Copy link
Owner

mhart commented Sep 28, 2023

I'm confused about why this is a problem suddenly – the types are declared – why does anything need to change here? Is it a TypeScript bug?

@Cosmic-Ryver
Copy link

I'm confused about why this is a problem suddenly – the types are declared – why does anything need to change here? Is it a TypeScript bug?

In our project, the issue occurred after converting to ES Modules. As I understand, it is not a TypeScript bug. Depending on configuration in tsconfig.json and package.json TypeScript will strictly respect the exports field of dependencies' package.json files.

import /** ... */ from "dep/path" or import "dep/path" when "dep/path" is not explicitly included in dep's exports will be reported as an error. The absence of "./path" from exports is interpreted to mean the path is non-existent, private, or otherwise not a valid entry point. Similarly, import /** ... */ from "dep/path" when "./path" is included in exports, but does not have a types entry, will produce the error we encountered. In this case, any path.d.ts file is considered private because it is not explicitly exported.

The solution for compatibility with projects like our own is to either add the explicit types entry, or go with @tomlarkworthy's solution if fine grained control over what users are allowed to import is not desired.

Reproduction.

@tomlarkworthy
Copy link
Author

tomlarkworthy commented Sep 28, 2023

I think why now is Typescript 5 changed how it resolved. I think the motivation is that packages can distribute wildly different APIs per export, and thus need different types per export. Not sure why the top level type is not default though. I think @GusBuonv 's patch better captures there are not different type exports for this package

@revmischa
Copy link

I ran into this issue after switching from "moduleResolution": "node" to "moduleResolution": "bundler"

@mattiaz9
Copy link

Also "moduleResolution": "nodenext" creates this problem.
This comment explains why there is this issue:
microsoft/TypeScript#51996 (comment)

@Ehesp
Copy link

Ehesp commented Nov 29, 2023

I ran into this problem today as well. The exports field in package.json just needs a "types" entry.

Diff that solves the problem:

diff --git a/node_modules/aws4fetch/package.json b/node_modules/aws4fetch/package.json
index 4ea1e45..95842c7 100644
--- a/node_modules/aws4fetch/package.json
+++ b/node_modules/aws4fetch/package.json
@@ -14,7 +14,8 @@
       "worker": "./dist/aws4fetch.esm.js",
       "browser": "./dist/aws4fetch.umd.js",
       "require": "./dist/aws4fetch.cjs.js",
-      "default": "./dist/aws4fetch.umd.js"
+      "default": "./dist/aws4fetch.umd.js",
+      "types": "./dist/main.d.ts"
     }
   },
   "types": "dist/main.d.ts",

This was partially generated by patch-package.

You can't apply a patch via patch-package to a package.json file right?

EDIT: Oh, you can: npx patch-package aws4fetch --exclude 'nonthing' (ref)

@alfaproject
Copy link

I've just stumbled upon this issue as well after changing moduleResolution to bundler as we are in the process to convert from CommonJS to ESM ):

@mhart https://arethetypeswrong.github.io/?p=aws4fetch%401.0.17

@alfaproject
Copy link

Sorry for the notifications and off-topic but was just wondering if this is abandoned and I should fork or look for some other fork/alternative. Last commit in master was ~2 years ago and this is an actual/obvious issue that already has a PR with a fix for quite some time.

mhart added a commit that referenced this issue Feb 12, 2024
mhart added a commit that referenced this issue Feb 12, 2024
mhart added a commit that referenced this issue Feb 12, 2024
@mhart
Copy link
Owner

mhart commented Feb 12, 2024

This should be fixed in v1.0.18-beta.2 – please give it a spin and let me know if it fixes your issues

https://arethetypeswrong.github.io/?p=aws4fetch%401.0.18-beta.2

Will publish as ``v1.0.18` if it doesn't break anyone's workflows

@mhart mhart closed this as completed Feb 12, 2024
@alfaproject
Copy link

@mhart it works perfectly, thanks!

@mhart
Copy link
Owner

mhart commented Mar 3, 2024

Ok, feel like it's been enough time, lol. Have just published 1.0.18 (identical to 1.0.18-beta.2)

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

No branches or pull requests

7 participants