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

Add compatibility with TypeScript and ESM #429

Closed
wants to merge 1 commit into from

Conversation

jscheid
Copy link

@jscheid jscheid commented Mar 18, 2022

This adds compatibility with TypeScript's Experimental Support for ECMAScript Modules in Node.js. I've seen the package.json syntax mentioned in microsoft/TypeScript#33079 and it solves the issue for me.

It's meant as an example only, you might want to hold off merging this until TypeScript 4.7 is released. I'm not sure this will continue to work in the release. And if it does, you might want to do something similar for all the other packages in this repository. I've marked it as draft for these reasons.

Also, I haven't checked whether the syntax in index.d.mts would work with other TypeScript versions and configurations. If it does then it would be better to use a single file instead of both index.d.ts and index.d.mts.

And finally, I haven't checked whether TypeScript 4.7 without ESM support enabled also needs the different syntax. If so, it might be necessary to use the typesVersion setting.

Fixes #428

@angus-c
Copy link
Owner

angus-c commented Mar 19, 2022

Thanks @jscheid. I will hold onto this pending 4.7 release and checking the results in various environments (and pending me taking the time to understand it fully 😄 )

@adam-coster
Copy link

I just ran into this today since I'm using NodeNext for my Typescript setup. If I understand it all correctly, the crux of the issue is that the existence of the exports field makes it so that only the exports field will be used (and main, types, etc end up ignored). So in the case of this library:

  • There is an exports field, so the root-level types field is ignored by latest Typescript/Node versions
  • There is no exports['.'].types field, so Typescript falls back to look for a sibling file to the imported one (e.g. for index.js it looks for index.d.ts, while for index.mjs it looks for index.d.mts).
  • There is no sibling index.mjs file, so Typescript gives up trying to find types.

The current PR solves this by adding that index.d.mts file, and also adding that to the exports values, but there is a simpler solution: just add the current types filepath to the exports['.'].types field of the package.json:

{
  "exports": {
    ".": {
      // Just need to add the following line
      "types": "./index.d.ts",
      "require": "./index.js",
      "default": "./index.mjs"
    }
  }
}

In essence, that fixes the fact that the root-level types field gets ignored when the exports field is present.

@adam-coster
Copy link

Here's the diff I got when using patch-package to fix this locally:

diff --git a/node_modules/just-pick/package.json b/node_modules/just-pick/package.json
index 50223ce..256d586 100644
--- a/node_modules/just-pick/package.json
+++ b/node_modules/just-pick/package.json
@@ -7,7 +7,8 @@
   "exports": {
     ".": {
       "require": "./index.js",
-      "default": "./index.mjs"
+      "default": "./index.mjs",
+      "types": "./index.d.ts"
     }
   },
   "types": "index.d.ts",

@angus-c
Copy link
Owner

angus-c commented Jul 10, 2022

Hi @adam-coster. Sorry for the belated reply. This does sound like the best solution.
I will create a new PR that batches this change for all modules (unless someone else does it first 😉 )
Thanks @adam-coster and @jscheid for investigating!

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.

Could not find a declaration file for module 'just-omit' with Typescript 4.7
3 participants