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

Support Typescript's moduleResolution: "node16" #2307

Closed
amiller-gh opened this issue Jun 29, 2022 · 16 comments · Fixed by #2555
Closed

Support Typescript's moduleResolution: "node16" #2307

amiller-gh opened this issue Jun 29, 2022 · 16 comments · Fixed by #2555
Assignees
Labels
bundling Anything related to issues with bundling typescript
Milestone

Comments

@amiller-gh
Copy link

amiller-gh commented Jun 29, 2022

This is a bug in at least @turf/envelope@6.5.0 and potentially other packages.

Typescript has recently released a new moduleResolution option that when set to node16 or nodenext will use new ESM resolution logic. This includes an understanding of the new "exports" package.json field.

Any module in this monorepo that currently uses the exports field (like @turf/envelope) must now specify the types in the export config as well, otherwise Typescript will complain that it can't find the types file if you turn on moduleResolution: "node16".

E.g. Turn this:

{
  "main": "dist/js/index.js",
  "module": "dist/es/index.js",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "import": "./dist/es/index.js",
      "require": "./dist/js/index.js"
    }
  },
  "types": "index.d.ts",
}

Into this:

{
  "main": "dist/js/index.js",
  "module": "dist/es/index.js",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "types": "./index.d.ts",
      "import": "./dist/es/index.js",
      "require": "./dist/js/index.js"
    }
  },
  "types": "index.d.ts",
}

Until then, using this package in Typescript with the new module resolution scheme results in the error TS7016: Could not find a declaration file for module '@turf/envelope'.

@JamesLMilner
Copy link
Collaborator

Hey @amiller-gh, I think it's fair to say we're keen to support TypeScript as best we can (we are currently slowly converting Turf modules to TypeScript). This seems reasonably straight forward to resolve - do you have any links to any surrounding information/context? I've looked at the TypeScript page on moduleResolution but I didn't see any mention of the exports fields/types.

@yukha-dw
Copy link

+1, seems like TypeScript doesn't know where the typings is when using ES Module.

@nelson6e65
Copy link

This seems reasonably straight forward to resolve - do you have any links to any surrounding information/context? I've looked at the TypeScript page on moduleResolution but I didn't see any mention of the exports fields/types.

exports.x.types seems to be a community definition:

"types" - can be used by typing systems to resolve the typing file for the given export. This condition should always be included first.
https://nodejs.org/docs/latest-v16.x/api/packages.html#community-conditions-definitions

Firebase Admin uses it in its exports packaje.json key in addition to types:
https://github.com/firebase/firebase-admin-node/blob/v11.0.1/package.json#L112-L128
https://github.com/firebase/firebase-admin-node/blob/v11.0.1/package.json#L62

AngularFire seems to use typings key instead:
https://github.com/angular/angularfire/blob/7.4.1/package.json#L131

Chalks ES uses only types, but the type definition is in the same directory of the main exports:
https://github.com/chalk/chalk/blob/v5.0.1/package.json#L18

@nelson6e65
Copy link

I tried to hard-code my node_modules/@turf/turf/package.json adding types to tests this. It seems to "work", but I got a bunch of "Could not find a declaration file for module '@turf/XXXXX'" errors. 😅

@bonesoul
Copy link

bonesoul commented Jan 9, 2023

import { centerOfMass } from "@turf/turf";

getting the error declaration not found. any solutions?

@deloreyj
Copy link

@JamesLMilner the documentation you're looking for to fix this issue is here in the Typescript docs. It should be as simple as adding a types field to the exports

@evgenyt1
Copy link
Contributor

evgenyt1 commented Feb 3, 2023

Adding "types": "./index.d.ts" fixes things for me temporarily. But this should be done on @Turf side.

"exports": {
  "./package.json": "./package.json",
  ".": {
    "types": "./index.d.ts",
    "import": "./dist/es/index.js",
    "require": "./dist/js/index.js"
  }
},

@evgenyt1
Copy link
Contributor

I made a PR on this. Pretty straightforward.

@evgenyt1
Copy link
Contributor

I made a temp patch usable through pnpm's patchedDependencies and similar
@turf__turf@6.5.0.patch

@favna
Copy link
Contributor

favna commented Nov 21, 2023

An alternative patch that will also partially satisfy https://github.com/arethetypeswrong/arethetypeswrong.github.io. I also highly recommend @Turf using ATTW CLI or website to verify that their bundle works properly with both CJS and ESM. Usage is pretty dead simple, see the readme on their repo.

Note that the patch below is a partial satisfaction because officially you're supposed to ship a different types file for CJS as opposed to ESM, i.e. index.d.cts and index.d.ts/index.d.mts. This is related to how for CJS you would likely use export = MyPackage for proper interop with const MyPackage = require('my-package') whereas for ESM you would use export default MyPackage for proper interop with import MyPackage from 'my-package';

I'm also going to summon @Andarist and @andrewbranch here whom are both experts in the field of properly defining ESM/CJS/types and have helped me understand so much about this whole topic.

diff --git a/package.json b/package.json
index 1946d013646f884c74f2d5c7bf94d282154accdc..17cdc18d1858018ea9d41190c7ba53da3788b6d8 100644
--- a/package.json
+++ b/package.json
@@ -48,8 +48,14 @@
   "exports": {
     "./package.json": "./package.json",
     ".": {
-      "import": "./dist/es/index.js",
-      "require": "./dist/js/index.js"
+      "import": {
+        "types": "./index.d.ts",
+        "default": "./dist/es/index.js"
+      },
+      "require": {
+        "types": "./index.d.ts",
+        "default": "./dist/js/index.js"
+      }
     }
   },
   "browser": "turf.min.js",

@Andarist
Copy link

The proposed patch above doesn't look correct to me because it tries to reuse the same declaration file for both formats. A single file can only be interpreted either as a module or as a script. You want two files (often even with the same content) so they can independently represent your module (import condition) and your script (require condition). A single file can't be used for both without issues because it's ambiguous. In this particular example, it would be interpreted as a script because this package.json doesn't set "type": "module" so all .js files in this "scope" (and .d.ts runs on the same rules as .js) are treated as scripts.

You could try to copy over your index.d.ts to those es and js directories and point types condition to those copies. This could work because then that /dist/es/index.d.ts would be located in a directory that has an extra package.json in it, one that sets "type": "module". So that would make that .d.ts to be interpreted correctly as a module - whereas the .d.ts in the other directory would continue to represent the script.

@smallsaucepan smallsaucepan self-assigned this Dec 4, 2023
@smallsaucepan smallsaucepan added the bundling Anything related to issues with bundling label Dec 9, 2023
@smallsaucepan
Copy link
Member

Hi all. Incoming PR soon to hopefully address this issue and more. Would appreciate your input on the direction. Aiming to support both CJS and ESM.

Please have a look at the below and let me know (in theory) how it looks:

tsconfig.shared.json:

{
  "compilerOptions": {
    "target": "es2017",
    "module": "node16",
    "declaration": true,
    "esModuleInterop": true,
    "strict": true,
    "moduleResolution": "node16",
    "importHelpers": true,
    "skipLibCheck": true
  }
}

Contents of dist/ directory in each package generated by tsup:

  • index.js
  • index.d.ts
  • index.mjs
  • index.d.mts

The two d.ts files are identical, though duplicated as trying to reuse one for both CJS and ESM seems to be the source of some problems.

package.json extract

  "main": "dist/index.js",
  "module": "dist/index.mjs",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "require": "./dist/index.js",
      "import": "./dist/index.mjs"
    }
  },

No "type": "module" config item so CJS is default. No explicit d.ts entry points defined - taking a less is more approach and instead leaving it to the tooling or IDE to infer based on the location of the implementation file.

Retro-testing a few existing issues seems promising so far. Any obvious gotchas or oddities?

@favna
Copy link
Contributor

favna commented Dec 16, 2023

No explicit d.ts entry points defined - taking a less is more approach and instead leaving it to the tooling or IDE to infer based on the location of the implementation file.

I would highly recommend against this. It's really easy to add the entrypoints and there is 0 downsides. Refer to my comment above for how to define .d.ts files for both import and require.

I have recently fixed up all of the Sapphire projects that I maintain so you can also take a looksie here

@smallsaucepan
Copy link
Member

Thanks @favna. Really appreciate it. Based on your advice I will:

  • re-split dist into esm/ and cjs/ subdirs. Probably just my personal preference for avoiding too many hierarchy layers creeping in
  • include in the package.json exports config d.ts entry points

I notice in Sapphire package.json you don't set the type explicitly to commonjs (implied as the default node interpretation currently yes?). What do you think about the advice at the end of this topic to always declare the module type explicitly? Any reason not to?

This flag currently defaults to "commonjs", but it may change in the future to default to "module".

smallsaucepan added a commit to smallsaucepan/turf that referenced this issue Dec 16, 2023
…h d.ts entries. Turfjs#2307 (comment)  Specifying overall module type as commonjs explicitly.
@favna
Copy link
Contributor

favna commented Dec 16, 2023

Hmm so there is a very basic reason I don't have that and that's that I just didn't properly look into it yet.

That said, similar to the monorepos sapphiredev/utilities and sapphiredev/plugins I also maintain https://github.com/skyra-project/archid-components/ which while being primarily ESM only (some of the packages don't even support CJS), all packages do have "type": "module" so maybe it can be added for other packages as well. A small word of warning though, if you set "type": "module" then tsup will read this and change some of the file extensions. You can see here https://github.com/skyra-project/archid-components/blob/main/packages/env-utilities/package.json that .d.ts is then used for the ESM types and the CJS types get .d.cts as opposed to respectively .d.mts and .d.ts. Setting "type": "commonjs" however retains the types are you already applied them because as you quote, that is the current default.

The one thing that bothers me a bit with setting "type": "commonjs" is when the following tsconfig options are configured:

{
  "compilerOptions": {
    "module": "node16",
    "moduleResolution": "node16",
    "verbatimModuleSyntax": true,
    "noEmit": true,
    "strict": true
  }
}

Now running tsc on the code (for type checking only, this is part of our CI/CD pipeline) will throw many of these kinds of errors:

src/scheduled-tasks/postStats.ts:4:10 - error TS1286: ESM syntax is not allowed in a CommonJS module when 'verbatimModuleSyntax' is enabled.

4 import { Status } from 'discord.js';
           ~~~~~~

src/scheduled-tasks/postStats.ts:12:1 - error TS1287: A top-level 'export' modifier cannot be used on value declarations in a CommonJS module when 'verbatimModuleSyntax' is enabled.

12 export class PostStatsTask extends ScheduledTask {

In a sense valid because the package.json is saying you're writing CJS code, but annoying nonetheless. Now the fix for that is easy and that is to change to tsconfig to:

{
  "compilerOptions": {
-    "module": "node16",
-    "moduleResolution": "node16",
+   "module": "es2022",
+   "moduleResolution": "bundler",
    "verbatimModuleSyntax": true,
    "noEmit": true,
    "strict": true
  }
}

which indicates to TypeScript that you're compiling with a bundler (i.e. tsup -> esbuild) so maybe I should just fully embrace this and set the field anyway but I would still want to do some proper research into if it has any other unforeseen downsides.


Lastly regarding this point:

re-split dist into esm/ and cjs/ subdirs. Probably just my personal preference for avoiding too many hierarchy layers creeping in

Truth be told I'm not the biggest fan either but I'm even less a fan of mixing CJS and ESM files in the same directory (and our @sapphire/framework would go completely badonkadonk if it would because it reads files from the file system) so as long as we're compiling for 2 ecosystems I'd say it's the best solution we got.

And as for this one:

include in the package.json exports config d.ts entry points

Yeah I totally get it. I just never had any request from a community member or reason otherwise to expose the package.json that way. As far as getting the version goes I created an esbuild plugin (usable with tsup) for that purpose and it serves me and other users well https://github.com/favware/esbuild-plugin-version-injector

@smallsaucepan
Copy link
Member

Thanks @favna. Would definitely like to enable verbatimModuleSyntax down the track. Aiming to also add arethetypeswrong to the CI pipeline too.

Would you mind casting an eye over PR #2555? If there aren't any glaring deficiencies we'll aim to release the next alpha once that's merged.

smallsaucepan added a commit that referenced this issue Dec 18, 2023
* Moving to pnpm as package manager. Also taking the opportunity to tidy up our cjs / esm situation using tsup (instead of tsc and rollup). Can't get monorepolint to work with new setup so disabling for the time being.

* Changing all Turf imports to use named imports. Adding a few missing named exports. Updating github workflow actions to pnpm instead of yarn.

* Updating typescript module and moduleResolution mechanisms to "node16". Not tracking nodenext just yet. Changing tsup command to generate d.ts files that arethetypeswrong approves of.

* Tweaking github workflows to explicitely install pnpm.

* Looks like node needs the chosen cache binary installed first. Trying without cache as a first step.

* Seem to be hitting the tsx bug described here: privatenumber/tsx#421 Upgrading to latest tsx to remedy.

* Forgot to update lock file after updating tsx dep in package.json files.

* Sorting modules exported from @turf/turf alphabetically. Preparatory commit to add unexported modules.

* Converting index.mjs to typescript, and simplifying re-exported imports, and ordering alphabetically. Added in a couple of packages - convex, booleanValid and nearestNeighbourAnalysis. Rollup now only used in packages/turf so merging project root base config into there. Also now only using rollup in packages/turf to do final conversion to web module. JS and d.ts files generated the same as other modules, using tsup.

* Including tslib in @turf/turf build now that it's a TS module.

* Lot of per-package tsconfig items now on the tsup command line, so stripping back tsconfig.json to the bare minimum. Adding tsconfig.json to packages that while still only JS, are now processed by tsup.

* Same as last commit - simplifying tsconfig.json - except these are the subset of packages that had multiple entry points e.g. 3rd party code hosted locally in lib/

* Incorporating some suggestions from @favna on being more explicit with d.ts entries. #2307 (comment)  Specifying overall module type as commonjs explicitly.

* Reapplying cjsInterop / splitting workaround for CJS exports that we had on build target command line earlier in this PR in to tsup config file. Also disabling treeshaking as this was generating a warning for CJS files and causing the workaround above to not work.

* Upgrading rollup and related plugins. Updating to recommended modern browserslist config in babel.config.json e.g. previous chrome 67 and edge 17 are 5 years old, ie11 is just ... ugh

* Fluffed the browserslist config - need to cover es5 as well.

* Update .github/workflows/turf.yml to use actions/checkout v4.

Co-authored-by: Jeroen Claassens <jeroen.claassens@live.nl>

---------

Co-authored-by: Jeroen Claassens <jeroen.claassens@live.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bundling Anything related to issues with bundling typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants