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 issues with moduleResolution: "nodenext" and declaration: true #18

Closed
1 of 2 tasks
eugene1g opened this issue Feb 15, 2023 · 19 comments · Fixed by #27
Closed
1 of 2 tasks

TypeScript issues with moduleResolution: "nodenext" and declaration: true #18

eugene1g opened this issue Feb 15, 2023 · 19 comments · Fixed by #27
Labels
bug Something isn't working

Comments

@eugene1g
Copy link

Guidelines

  • Please search other issues to make sure this bug has not already been reported.

Describe the bug

When using TypeScript with "moduleResolution: "nodenext" and "declaration": true", it fails to compile the example from README

Steps to reproduce

main.ts

import ModernError from 'modern-errors'
export const BaseError = ModernError.subclass('BaseError')

tsconfig.json

{
  "compilerOptions": {
    "target": "esnext",
    "module": "esnext",
    "moduleResolution": "nodenext",
    "declaration": true
  }
}

error:

main.ts:3:14 - error TS2742: The inferred type of 'BaseError' cannot be named without a reference to './node_modules/modern-errors/build/src/subclass/create.js'. This is likely not portable. A type annotation is necessary.

3 export const BaseError = ModernError.subclass('BaseError')
               ~~~~~~~~~

main.ts:3:14 - error TS2742: The inferred type of 'BaseError' cannot be named without a reference to './node_modules/modern-errors/build/src/subclass/custom.js'. This is likely not portable. A type annotation is necessary.

3 export const BaseError = ModernError.subclass('BaseError')
               ~~~~~~~~~


Found 2 errors in the same file, starting at: main.ts:3

I can make it work in two ways -

  1. Disable declaration. However, this means my library cannot publish types and that's not great.
  2. Change to "moduleResolution": "node". However, this means I cannot use the exports settings in my dependencies.

With "moduleResolution":"node" it generates these types for main.d.ts:

export declare const BaseError: import("modern-errors/build/src/subclass/create").ErrorSubclassCore<[], {}, import("modern-errors/build/src/subclass/custom").CustomClass>;

Which hints at the reason for failure: TypeScript has to tap into internal files to reference un-exported types to get ErrorSubclassCore (and, in my real project, CustomClass). With "moduleResolution":"node", this deep linking is not possible, so the TS compiler gives up.

Environment

  System:
    OS: macOS 13.1
    CPU: (8) arm64 Apple M1
    Memory: 64.61 MB / 8.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.14.0 - ~/.asdf/installs/nodejs/18.14.0/bin/node
    Yarn: 1.22.19 - ~/.asdf/shims/yarn
    npm: 9.3.1 - ~/.asdf/plugins/nodejs/shims/npm

Pull request (optional)

  • I can submit a pull request.
@eugene1g eugene1g added the bug Something isn't working label Feb 15, 2023
@eugene1g eugene1g changed the title TypeScript option moduleResolution: "nodenext" breaks types TypeScript issues with moduleResolution: "nodenext" and declaration: true Feb 15, 2023
@ehmicky
Copy link
Owner

ehmicky commented Feb 15, 2023

Hi @eugene1g,

Thank for reaching out.

I am having issues reproducing this. Which version of TypeScript are you using?

Also, would switching compilerOptions.module from esnext to nodenext fix your problem?

@eugene1g
Copy link
Author

Hi @ehmicky

I forgot to say "thank you!" for the awesome library.

Changing module to nodenext still has the same result. I'm trying this with the latest TypeScript 4.9.5 and modern-errors 5.5.1. Here's a test repo https://github.com/eugene1g/modern-terrors

@eugene1g
Copy link
Author

eugene1g commented Mar 1, 2023

The core issue here is when the application uses moduleResolution: nodenext (or node16), TypeScript (and Node) is not allowed to tap into any arbitrary files inside the library packages. Under modeResolution: node, this is allowed because it's just referencing a file on the filesystem. With modeResolution: node16, the boundary of the library is respected and the application can import only what is specifically exported by the library. modern-errors needs to export types of CustomClass and ErrorSubclassCore so they are accessible by TS from the user's application.

This patch will do that - it tweaks modern-errors to export the two missing types, and solves the issue:

diff --git a/src/main.d.ts b/src/main.d.ts
index 2da7b22..1ce8f22 100644
--- a/src/main.d.ts
+++ b/src/main.d.ts
@@ -7,6 +7,8 @@ export type { InstanceOptions } from './options/instance.js'
 export type { MethodOptions } from './options/method.js'
 export type { Info } from './plugins/info/main.js'
 export type { Plugin } from './plugins/shape/main.js'
+export type { CustomClass } from './subclass/custom.js'
+export type { ErrorSubclassCore } from './subclass/create.js'
 export type { ErrorClass }
 
 /**
diff --git a/src/subclass/create.d.ts b/src/subclass/create.d.ts
index 3d6a65e..02712a0 100644
--- a/src/subclass/create.d.ts
+++ b/src/subclass/create.d.ts
@@ -20,7 +20,7 @@ import type { NormalizeError } from './normalize.js'
 /**
  * `ErrorClass.subclass()`
  */
-type CreateSubclass<
+export type CreateSubclass<
   PluginsArg extends Plugins,
   ErrorPropsArg extends ErrorProps,
   CustomClassArg extends CustomClass,

@ehmicky
Copy link
Owner

ehmicky commented Mar 14, 2023

Hi @eugene1g,

Sorry for the late reply.
Thank you so much for hinting at the solution, this really helped me out! This should be now fixed in 5.5.2.

@all-contributors Please add @eugene1g for code, bug.

@allcontributors
Copy link
Contributor

@ehmicky

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

I've put up a pull request to add @eugene1g! 🎉

@eugene1g
Copy link
Author

eugene1g commented Mar 18, 2023

Hi @ehmicky ,
Thank you for fixing this with 5.5.2 - the original thing works well now.

I started using Modern Error plugins, and discovered that we need one more type, CommonInfo. I updated the test repo to show the issue when using Plugins at https://github.com/eugene1g/modern-terrors

The fix would be to export CommonInfo from src/plugins/info/main.d.ts and expose it publically in src/main.d.ts:

// https://github.com/ehmicky/modern-errors/blob/516a73948a8eb31cb21c449d226fbf3baf6f3965/src/main.d.ts#L12
 export type { CommonInfo, Info } from './plugins/info/main.js'

@ehmicky
Copy link
Owner

ehmicky commented Mar 18, 2023

Hi again @eugene1g,

Thanks for reporting this too! 👍
This should be fixed in 5.5.4.

@jmchambers
Copy link
Contributor

jmchambers commented Mar 31, 2024

Hi, it might just be something weird about my setup but I'm getting the same TS2742 error when using the custom option:

export const UserError = BaseError.subclass('UserError', {
  custom: class extends BaseError {
    constructor(message: string, options?: InstanceOptions & UserErrorOptions) {
      // do stuff...
      super(message, options)
    }
  },
})

Exporting a couple more types, CreateSubclass and NormalizeError, fixes the issue. Here's the pnpm patch I'm using:

diff --git a/build/src/main.d.ts b/build/src/main.d.ts
index a17f0e34534a9a5c1a8fe2c55bc2ef964f65ef8b..63413eab64ab41b933a6f39f731b00a12e86efc4 100644
--- a/build/src/main.d.ts
+++ b/build/src/main.d.ts
@@ -2,6 +2,7 @@ import type {
   ErrorClass,
   SpecificErrorClass,
   ErrorSubclassCore,
+  CreateSubclass,
 } from './subclass/create.js'
 import type { CustomClass } from './subclass/custom.js'
 
@@ -22,7 +23,9 @@ export type {
   // https://github.com/ehmicky/modern-errors/issues/18
   CustomClass,
   ErrorSubclassCore,
+  CreateSubclass,
 }
+export type { NormalizeError } from './subclass/normalize.js'
 
 /**
  * Top-level `ErrorClass`.
diff --git a/build/src/subclass/create.d.ts b/build/src/subclass/create.d.ts
index 3d6a65eebd807c761e23bb0e5065b0c6c6063fa6..02712a03c1e41a4aa8aa9d687a8b981cb9b2a92d 100644
--- a/build/src/subclass/create.d.ts
+++ b/build/src/subclass/create.d.ts
@@ -20,7 +20,7 @@ import type { NormalizeError } from './normalize.js'
 /**
  * `ErrorClass.subclass()`
  */
-type CreateSubclass<
+export type CreateSubclass<
   PluginsArg extends Plugins,
   ErrorPropsArg extends ErrorProps,
   CustomClassArg extends CustomClass,

Please let me know if you want me to create a new issue with a reproduction and/or create a pull request. Thanks.

@ehmicky
Copy link
Owner

ehmicky commented Mar 31, 2024

Hi @eugene1g,

Thanks for reaching out.
Could you please create a reproduction in the TypeScript playground please?

@jmchambers
Copy link
Contributor

jmchambers commented Apr 1, 2024

Sure, @ehmicky, here's the repro link. Please note, however, that the link doesn't recreate the error when I first visit it, which is weird, I have to restore the config shown in the pics below, and toggle declarations on and off, then the lint error eventually appears:

Screenshot 2024-04-01 at 02 07 37 Screenshot 2024-04-01 at 02 07 15

UPDATE: this stackblitz link shows the problem more reliably:

Screenshot 2024-04-01 at 02 59 54

Seems to be the same issue, whereby declaration: true + modern module resolution, throws TS2742 for any types used in type inference that aren't explicitly exported by modern-errors main.d.ts.

Great library BTW, my errors have never been so nicely organised!

@ehmicky ehmicky reopened this Apr 7, 2024
@ehmicky
Copy link
Owner

ehmicky commented Apr 7, 2024

Thanks a lot for reporting this, that's very helpful!

I can reproduce this problem, and can see that your fix would work. What I'm trying to figure out is: why is this currently failing? So this problem does not appear again in the future.

Also, exporting types that are not meant to be public, as a workaround for a bug, might not be as good as understanding the underlying problem, and fixing that instead.

The project has a wide array of type tests, especially on the custom option. It even includes the same code as above.

ModernError.subclass('TestError', {
custom: class extends ModernError {
constructor(message: string, options?: InstanceOptions) {
super(message, options)
}
},
})

Changing the tsconfig.json to the same as above does not trigger tsd (the type test runner we're using) to fail. 🤔

Would you like to submit a PR with your above fix?
If possible, would you be able to first make the type tests fail, so that they work as regression tests for this bug?

Thanks again!

@jmchambers
Copy link
Contributor

jmchambers commented Apr 7, 2024

Hi @ehmicky, it is probably related to this open issue on TS itself:

microsoft/TypeScript#47663

It's a very weird error in that a @ts-ignore comment can't remove it, and so I'm not surprised that tsd is having problems detecting it!

It might be worth waiting to see what the official solution for that TS issue is as, I agree, exposing private types is less than ideal, and it'll be a never ending problem. For example, I was messing around extending the inferred modern-errors type in various other ways and I encountered the same TS2742 issue, with the fix being to expose yet more private types!

That said, I haven't encountered this error with other packages, so perhaps there is something odd about the modern-errors setup? Might the title of the TS issue be a clue: "... occurs when multiple modules with the same package ID are resolved"? When I look in node_modules I can see that there are multiple entries for the packages that modern-errors depends on, e.g., merge-error-cause:

modern-errrors-package-duplicates

Might a solution be to figure out why the packages are installed both at the root-level and in the nested node_modules folders? I'm a newbie at Node & TS, so not sure. What do you think...?

@ehmicky
Copy link
Owner

ehmicky commented Apr 7, 2024

Very good find, this does appear to be the TypeScript issue!

Modern errors uses plugins like modern-errors-bugs. Each of those plugins has modern-errors as a peer dependency. This is to guarantee that the plugins use the right version of modern-errors they are compatible with.

Then, modern-errors installs all plugins as dev dependencies to be able to run the following type test.

import ModernError, { type ErrorInstance } from 'modern-errors'
import bugsPlugin from 'modern-errors-bugs'
import cleanPlugin from 'modern-errors-clean'
import cliPlugin from 'modern-errors-cli'
import httpPlugin, { type HttpResponse } from 'modern-errors-http'
import processPlugin from 'modern-errors-process'
import serializePlugin, { type ErrorObject } from 'modern-errors-serialize'
import switchPlugin from 'modern-errors-switch'
import winstonPlugin, { type Format } from 'modern-errors-winston'
import { expectAssignable, expectType } from 'tsd'

Since npm (and pnpm apparently?) installs peer dependencies, this means modern-errors installs itself. However, it only happens in development. Normal users do not install dev dependencies, so there is no duplication for them.

Do you think the following fix would work?

@jmchambers
Copy link
Contributor

Yeah, I tried to use that fix before reporting the issue to you, but I couldn't get it to work. I might have misunderstood how to use the fix though. I tried making all the modern-errors deps, deps of my own project, and then importing their types like so:

import type {} from 'error-class-utils'
import type {} from 'error-custom-class'
import type {} from 'filter-obj'
import type {} from 'is-plain-obj'
import type {} from 'merge-error-cause'
import type {} from 'normalize-exception'
import type {} from 'set-error-message'
import type {} from 'set-error-props'
import type {} from 'set-error-stack'

That didn't work though. How would you interpret the fix?

@ehmicky
Copy link
Owner

ehmicky commented Apr 7, 2024

I was also unclear about what they meant.

Modern errors is definitely doing some weird shenanigans. The types are quite complex and highly recursive.

I wrote the following comments summarizing it.

// Major limitations of current types:
// - Plugin methods cannot be generic
// - Plugin types can use `ErrorClass` or `Info`, but not export them.
// See the comment in info.d.ts for an explanation.
// Medium limitations:
// - Some logic relies on determining if an error class is a subclass of
// another
// - However, this is not perfectly possible with TypeScript since it is
// based on structural typing
// - Unrelated classes will be considered identical if they have the same
// options
// - The `props` and `plugins` option do manage to create proper
// inheritance, but not the `custom` option
// - This impacts:
// - `ErrorClass.normalize()` second argument might not always fail when
// it is not a subclass of `ErrorClass`
// - `ErrorClass.normalize(new ErrorClass(''), ErrorSubClass)` returns
// an instance of `ErrorClass` instead of `ErrorSubclass`
// - If two `plugin.properties()` (or `props`) return the same property, they
// are intersected using `&`, instead of the second one overriding the first
// - Therefore, the type of `plugin.properties()` that are not unique should
// currently be wide to avoid the `&` intersection resulting in
// `undefined`
// - This problem does not apply to error core properties (`message` and
// `stack`) which are always kept correct
// - Type narrowing with `instanceof` does not work if there are any plugins
// with instance|static methods. This is due to the following bug:
// https://github.com/microsoft/TypeScript/issues/50844
// - When a `custom` class overrides a plugin's instance method, it must be
// set as a class property `methodName = (...) => ...` instead of as a
// method `methodName(...) { ... }`. This is due to the following bug:
// https://github.com/microsoft/TypeScript/issues/48125
// - When a `custom` class overrides a core error property, a plugin's
// `instanceMethods`, `properties()` or `props`, it should work even if it is
// not a subtype of it
// - `ErrorClass.subclass(..., { custom })` should fail if `custom` is not
// directly extending from `ErrorClass`, but it currently always succeed
// except when either:
// - `custom` class is not a `ModernError`
// - `ErrorClass` (or a parent) has a `custom` class itself
// - Defining the same plugin twice should fail, but it is a noop instead
// Minor limitations:
// - Plugin instance|static methods should not be allowed to override `Error.*`
// (e.g. `prepareStackTrace()`)
// - Plugins should not be allowed to define instance|static methods already
// defined by other plugins

One key problem is that modern-errors extend Error, so one would think class ... extends Error should be used. However, it can't due to some TypeScript limitations, which leads to doing some unusual prototype and constructor typing to emulate class ... extends Error, and passing error classes around as generic types.

It is a shame though to not be able to use the custom option with TypeScript. Do you find a way to at least suppress the type error?

@jmchambers
Copy link
Contributor

Yeah, you're definitely using some TS voodoo magic that goes right over my head!

My guess is that the problem is related to that underlying TS issue, and that modern-errors is susceptible to it because of:

  • that TS voodoo, creating an uncommon failure mode that other packages don't typically run into
  • or something odd about the way modern-errors uses those related packages, though I've no idea what
  • or some combination of the two

I'm not sure I can usefully contribute to a fix as I'm a bit out of my depth! But I do have a workaround that works for me, and that's simply to use PNPM's built in patching mechanism to add exports of any private types that are causing problems. I guess other TS users will just have to do the same if they want to use the custom option, until that underlying TS issue has a fix, or a better workaround 🤷.

Either that, or I can submit a PR to add those exports with a comment making it clear that they're only exported as a temporary measure until the underlying TS issue has a fix. I could export all the private types individually, with a JSDoc comment that'll mark them as deprecated in IDE tooling, e.g.:

import type { NormalizeError } from './subclass/normalize.js'

/**
 * @deprecated This type is private and only exported as a temporary workaround
 * for an open issue with TypeScript (see {@link https://github.com/microsoft/TypeScript/issues/47663|TypeScript issue #47663}).
 * It will be removed in a future release.
 */
export type NormalizeError

Your call, happy to raise the PR.

@ehmicky
Copy link
Owner

ehmicky commented Apr 7, 2024

Yes, it does seem to be the way you describe it.
I actually found a couple of other TypeScript bugs as part of writing the types for this package. :)

If you'd like to submit the above fix, that'd be great!

@ehmicky
Copy link
Owner

ehmicky commented Apr 8, 2024

Released in 7.0.1. 🎉

@jmchambers
Copy link
Contributor

Released in 7.0.1. 🎉

Installed and working! Nice doing business with you! 🙏

kodiakhq bot pushed a commit to cloudquery/plugin-sdk-javascript that referenced this issue May 1, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [modern-errors](https://togithub.com/ehmicky/modern-errors) | dependencies | patch | [`7.0.0` -> `7.0.1`](https://renovatebot.com/diffs/npm/modern-errors/7.0.0/7.0.1) |

---

### Release Notes

<details>
<summary>ehmicky/modern-errors (modern-errors)</summary>

### [`v7.0.1`](https://togithub.com/ehmicky/modern-errors/blob/HEAD/CHANGELOG.md#701)

[Compare Source](https://togithub.com/ehmicky/modern-errors/compare/7.0.0...7.0.1)

#### Types

-   Fix TypeScript types when using the `custom` option
 [ehmicky/modern-errors#18).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on the first day of the month" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMzAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjMzMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJhdXRvbWVyZ2UiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants