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 errors with loading type declaration files when TypeScript module resolution is set to Node16or NodeNext #999

Merged
merged 7 commits into from
Mar 18, 2023

Conversation

rotemdan
Copy link
Contributor

@rotemdan rotemdan commented Mar 17, 2023

The most significant error is:

Could not find a declaration file for module 'compromise'. 
'node_modules/compromise/src/three.js' implicitly has an 'any' type.

There are types at 'node_modules/compromise/types/three.d.ts', 
but this result could not be resolved when respecting package.json "exports". 
The 'compromise' library may need to update its package.json or typings.

Here is a relevant issue on the TypeScript repository (closed as "external")

Solution that seems to work:

In package.json I specified the types property for each individual export (with relative path):

  "exports": {
    ".": {
      "import": "./src/three.js",
      "require": "./builds/three/compromise-three.cjs",
      "types": "./types/three.d.ts"
    },
    "./tokenize": {
      "import": "./src/one.js",
      "require": "./builds/one/compromise-one.cjs",
      "types": "./types/one.d.ts"
    },
    "./one": {
      "import": "./src/one.js",
      "require": "./builds/one/compromise-one.cjs",
      "types": "./types/one.d.ts"
    },
    "./two": {
      "import": "./src/two.js",
      "require": "./builds/two/compromise-two.cjs",
      "types": "./types/two.d.ts"
    },
    "./three": {
      "import": "./src/three.js",
      "require": "./builds/three/compromise-three.cjs",
      "types": "./types/three.d.ts"
    }
  },

However, this didn't completely fix all the errors.

Another problem is that some definition files have .ts extensions, so I renamed the extensions of those files to .d.ts.

Another issue is that the internal reference in the definition files didn't include the full .d.ts extensions so I got errors like:

Relative import paths need explicit file extensions in EcmaScript imports 
when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

So I changed references like

import type View from './view/one'

to:

import type View from './view/one.d.ts'

I'm not sure if these changes are 100% backwards-compatible, since I renamed .ts files to .d.ts files (I don't know why they had the .ts extension). Also, I'm not sure how these changes would be compatible with older versions of TypeScript and node.js. (edit: I tried to make it backwards-compatible in further changes).

I decided to suggest this as a pull request, rather than an issue, because I already made these changes on my local copy of the library, and I wanted this to be resolved fast, to enable easy updates to future versions of the package. Please suggest or make any further changes you feel are needed.

@rotemdan
Copy link
Contributor Author

rotemdan commented Mar 18, 2023

When I tried to import compromise-dates in my TypeScript project (adding the new type definitions that haven't been published to the official npm package yet) I got the same error ("types should be referenced per export if there are multiple ones"). Since the plugins also use the exports property, I modified all plugins' package.json files to also use per-export typing references.

@rotemdan rotemdan changed the title Fix issues with loading TypeScript definition files in TypeScript 5.0 Fix errors with loading type declaration files when TypeScript module resolution is set to Node16or NodeNext Mar 18, 2023
@rotemdan
Copy link
Contributor Author

rotemdan commented Mar 18, 2023

After some consultation with Bing chat, I realized the top-level types field was necessary to preserve compatibility with versions of TypeScript earlier than 4.5. I tracked the original introduction of the feature in the October 1st, 2021 release of TypeScript 4.5 Beta, here is the example they originally gave:

// package.json
{
    "name": "my-package",
    "type": "module",
    "exports": {
        ".": {
            // Entry-point for `import "my-package"` in ESM
            "import": "./esm/index.js",

            // Entry-point for `require("my-package") in CJS
            "require": "./commonjs/index.cjs",

            // Entry-point for TypeScript resolution
            "types": "./types/index.d.ts"
        },
    },

    // CJS fall-back for older versions of Node.js
    "main": "./commonjs/index.cjs",

    // Fall-back for older versions of TypeScript <--- Note
    "types": "./types/index.d.ts"
}

The original package.json also had a typesVersions field, which I removed:

  "typesVersions": {
    "*": {
      "one": [
        "types/one.d.ts"
      ],
      "two": [
        "types/two.d.ts"
      ],
      "three": [
        "types/three.d.ts"
      ]
    }
  },

I looked up the documentation but I still don't know if it does anything with regards to multiple exports? I'll bring it back if needed, though I'm not sure if it had any effects.

Edit: I brought typesVersions field back, for now.

…mpatibility for old TS versions, although it is not needed on TypeScript 4.5+, and may not be 100% effective.
@spencermountain
Copy link
Owner

hey Rotem - thank you so much for this. Yes- I've heard that this exports.type is now recommended. Hilarious that Bing chat also agrees.
I am recovering from a head-cold, and may not be smart-enough to do this right now, but I'll merge this to dev this week and play around with it.

I've never felt that changing typescript is a major change - but yeah, people may complain, so we should do this carefully.
thank you for your help on this.

@spencermountain spencermountain changed the base branch from master to dev March 18, 2023 14:56
@spencermountain spencermountain merged commit 204df77 into spencermountain:dev Mar 18, 2023
@spencermountain
Copy link
Owner

changed my mind - LGTM. May bug you w/ some questions before next release.
thank you.

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