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: ESM default import This expression is not callable #230

Closed
wants to merge 8 commits into from

Conversation

yutak23
Copy link
Contributor

@yutak23 yutak23 commented Jul 5, 2023

This ought to fix #228

ESM seems incompatible in CommonJS PJs with default export (see microsoft/TypeScript#52086).
I think there are two solutions to make TypeScript types compatible with both CommonJS and ESM.

  1. Create a new type definition for ESM
    reference: https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing
  2. Change type definitions to support both CommonJS and ESM

Case 1, I would make a copy of the current index.d.ts, rename the file to index.d.mts, and change package.json as follows.

  "exports": {
    ".": {
      "import": {
        "types": "./index.d.mts",
        "default": "./lib/esm/index.js"
      },
      "require": {
        "types": "./index.d.ts",
        "default": "./index.js"
      }
    },
    "./package.json": "./package.json"
  }

However, I think this is redundant and low-maintainability because we have to manage two type definitions, so I tried to modify it in the second way(Case 2).

Explanation of this modification

  • By setting export = , both CommonJS and ESM are supported.
    I do not expect any impact on existing CommonJS implementations of TypeScript developers. Originally, when using export default, as described in Default Exports, the esModuleInterop option must be used, and testing has confirmed that it can be compiled with this setting.
    Also, testing confirms that the project can be compiled with the project in the ESM configuration.

  • dtslint and index.test-d.ts
    I'm adding dtslint and type definition tests to this project to match the axios project setup.
    The settings in tsconfig.json and tslint.json for dtslint are copied almost directly from axios, so there should be no problems.
    I think it should be included because the addition of dtslint allows us to safely update type definitions.
    (More than half of the modifications to existing type definitions is based on dtslint checks.)

For this change, I refer to the following

@mindhells
Copy link
Member

@yutak23 1st of all: thanks a lot for your effort and patience

I'm not completely sure about the approach. Although it seems to solve the typings problem, IMO it increases the cost of maintenance. At this point, wouldn't it make sense to port the project to TS?, so we also get rid of having to synchronise the TS definitions, WDYT?

I'd like to get more feedback on this from other maintainers

@yutak23
Copy link
Contributor Author

yutak23 commented Jul 30, 2023

@yutak23 1st of all: thanks a lot for your effort and patience

I'm not completely sure about the approach. Although it seems to solve the typings problem, IMO it increases the cost of maintenance. At this point, wouldn't it make sense to port the project to TS?, so we also get rid of having to synchronise the TS definitions, WDYT?

I'd like to get more feedback on this from other maintainers

@mindhells

Thank you for your comment.

I agree that it would require maintenance of both JavaScript and TypeScript types, and in that sense would increase maintenance costs.

I agree with you on the move to TypeScript.

@yutak23
Copy link
Contributor Author

yutak23 commented Aug 28, 2023

I have changed my PR content and raised another PR. This one is closed.

#241

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.

ESM default import This expression is not callable
2 participants