Skip to content

types: Export types in a way compatible with TypeScript "moduleResolution": "NodeNext"#200

Closed
brettwillis wants to merge 3 commits intofastify:masterfrom
brettwillis:fix-typings-for-esm
Closed

types: Export types in a way compatible with TypeScript "moduleResolution": "NodeNext"#200
brettwillis wants to merge 3 commits intofastify:masterfrom
brettwillis:fix-typings-for-esm

Conversation

@brettwillis
Copy link

@brettwillis brettwillis commented Oct 16, 2022

This is a typing issue only, not a runtime isse. Since TypeScript 4.8.3, when using "moduleResolution": "NodeNext" (or "Node16") for an ESM project, TypeScript will now report an error when using the code below:

import fp from 'fastify-plugin';

export default fp(...); // Error: This expression is not callable. Type 'typeof import(".../node_modules/fastify-plugin/plugin")' has no call signatures.

My own understanding is limited, but there has been a lot of discussion, for example microsoft/TypeScript#48845. Basically I think it is because the typings descibe the module as an ES module with a default export (which would be correct if the module was an ES module), but it is actually a CJS module exporing the plugin function with a default property...

Either way, I've tested this PR in an ESM project with "moduleResolution": "Node" and "moduleResolution": "NodeNext" (or "Node16") and it compiles successfully for both.

Checklist

@mcollina
Copy link
Member

@fastify/typescript could you take a look?

@fox1t
Copy link
Member

fox1t commented Oct 17, 2022

Oh. This could be a problem, and we should find a solution. @brettwillis, we export default (and map it in JS code) to support every environment, bundler, and compiler out there. We call it the "famous triplet" because it allows us to make Fastify modules compatible with ESM, faux ESM, TS, and CJS. Changing our types to "namespace" will make them usable only with "import * as something from 'something' under TS (or IDE that infers types for js files, such as VS Code) if "moduleInterop" is not set to true. Can you please make a repro repository I can take a look at? I want to understand if there are less radical solutions here.

@climba03003
Copy link
Member

@fox1t You may want to take a look at fastify/fastify-cookie#184 which is the similar approach.

@fox1t
Copy link
Member

fox1t commented Oct 17, 2022

@climba03003 I am starting to think that the era of the "famous triplet" is over. If so, we should update all of our types accordingly. As far as I understood from your link the issue is "NodeNext" + "type": "module", right?

@climba03003
Copy link
Member

I am starting to think that the era of the "famous triplet" is over.

Yes. and No. It still supported somehow, but it has more problem pop-up.
Including fastify/fastify-cors#231 (comment)

If so, we should update all of our types accordingly.

Yes

As far as I understood from your link the issue is "NodeNext" + "type": "module", right?

Yes, this combination is the major problem.

@fox1t
Copy link
Member

fox1t commented Oct 17, 2022

After further discussion with @climba03003 and basic repro on my machine, I would not like to make these types CJS (aka using export = syntax with the namespace). We should add explicit famous triplet to fp, instead, as suggested here fastify/fastify-cors#231 (comment)

@fox1t
Copy link
Member

fox1t commented Oct 17, 2022

P.S. I still want to see a repro repo here because I am unsure about the fix.
@brettwillis would you mind providing it, please?

Co-authored-by: KaKa <climba03003@gmail.com>
@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 17, 2022

Can we have a test for the last change?

@brettwillis
Copy link
Author

Sorry for the delay, will put together the repro tomorrow.

@brettwillis
Copy link
Author

Hi all, please see this branch in my fork https://github.com/brettwillis/fastify-plugin/tree/fix-typings-repros, and find the directories named:

  • types-repro-commonjs-new (this PR with CommonJS setup)
  • types-repro-commonjs-old (current typings with CommonJS setup)
  • types-repro-esm-nodenext-new (this PR with ESM/NodeNext setup)
  • types-repro-esm-nodenext-old (current typings with ESM/NodeNext setup)

The test setup looks like this

import fp1, { PluginOptions } from 'fastify-plugin';
import { default as fp2, PluginMetadata } from 'fastify-plugin';
import * as fp3 from 'fastify-plugin';

console.log(typeof fp1); // function
console.log(typeof fp1.default); // function
console.log(typeof fp2); // function
console.log(typeof fp2.default); // function
console.log(typeof fp3); // object
console.log(typeof fp3.default); // function


fp1(async () => {});
fp2(async () => {});

// esModuleInterop has no influence here, always type error and runtime error, so the typing is correct, this is consistent with old
// @ts-expect-error
fp3(async () => {}); // runtime TypeError: fp3 is not a function

In summary

  • The import fp from 'fastify-plugin' syntax always works at runtime (regardless of "esModuleInterop")
    • the current types cause an incorrect type error with "moduleResolution": "NodeNext" (incorrect because it works at runtime)
    • the new types fix the error
    • the new types also correctly identify the presence of the default property on the exported function which exists at runtime (but is missing in the current types)
  • The import * as fp from 'fastify-plugin' syntax only works with "esModuleInterop": false and is correctly represented by both the old and new types (with "esModuleInterop": true we must use fp.default)

Please double-check my work and let me know your thoughts.

@mcollina
Copy link
Member

@climba03003 @fastify/typescript I completely defer to you on this one.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the test in here within the test folder?
You can use tsd to assert if it provide the correct types for different tsconfig.json.
Or use tsc to see if the file compile correctly.

@brettwillis
Copy link
Author

@climba03003 apologies for the delay, it's pretty busy on my end. I've added tests for the the different import styles, but I'm unsure how to go about invoking with different tsconfig.json? So I believe for now tsd is just using "module": "commonjs", "moduleResolution": "node". Will try to figure something out in the coming days.

@Uzlopak Uzlopak mentioned this pull request Dec 1, 2022
4 tasks
@Uzlopak Uzlopak closed this Dec 3, 2022
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.

5 participants