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(types): fixed an issue with TypeScript not being able to locate .d.ts with moduleResolution: node16 #150

Merged

Conversation

Andarist
Copy link
Contributor

I've confirmed locally that it fixes the issue with such tsconfig.json:

{
  "compilerOptions": {
    "strict": true,
    "types": [],
    "moduleResolution": "Node16"
  }
}

See the comment by the TS member and the author of those new module resolutions here: microsoft/TypeScript#50890 (comment)

@pkerschbaum
Copy link
Contributor

pkerschbaum commented Sep 28, 2022

Oh I can see the issue, but the problem with the solution (adding the conditional export "types") reintroduces #144 (have confirmed that locally).
In other words, importing tiny-invariant from an ESM TS node16 codebase gets wrong typings again.

I think the reason is that now, with the conditional export "types", TypeScript always uses ./dist/tiny-invariant.d.ts for the types. Then TypeScript parses those as CJS (because it has the ambiguous file extension .ts and type of the "closest" package.json is not set).

I think this is how we should reconfigure exports, I confirmed that it does not reintroduce #144:

"exports": {
  ".": {      
    "import": "./dist/esm/tiny-invariant.js",
    "default": {
      "types": "./dist/esm/tiny-invariant.d.ts",
      "default": "./dist/tiny-invariant.cjs.js"
    }
  }
}

(we don't need to specify "types" for the ESM "import". Because TypeScript will automatically look for a file with identical basename but file extension .d.ts anyway, thus it correctly gets ./dist/esm/tiny-invariant.d.ts automatically).

Such nested conditions ("default" here) are absolutely valid, see nodejs.org/api/packages.html#nested-conditions and also the second example here: www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#packagejson-exports-imports-and-self-referencing.

What do you think @Andarist?

@pkerschbaum
Copy link
Contributor

I created the issue #151 for this so that folks running into the issue can find it.

@Andarist
Copy link
Contributor Author

@pkerschbaum could you push out a repo that would manifest the issue that you are talking about? I agree with everything that you have mentioned - I would just like to understand the issue itself better.

@alexreardon
Copy link
Owner

I will leave it to you two to let me know what to do on this one. I am out of my area of familiarity

@Andarist
Copy link
Contributor Author

Andarist commented Sep 28, 2022

I think I've found another issue with this - but one that was also present in 1.3.0. Because you are using exports: 'auto' in your Rollup config the generated CJS file looks like this:

function invariant(condition, message) { /* ... */ }
module.exports = invariant;

But all .d.ts that were/are there look like this:

export default function invariant(condition: any, message?: string | (() => string)): asserts condition;

The problem is that if we transpile the consuming project down to CJS and if that uses moduleResolution: node16 then we end up with this output:

"use strict";
exports.__esModule = true;
var tiny_invariant_1 = require("tiny-invariant");
console.log(tiny_invariant_1["default"]);

This is a potential runtime crash because at runtime there is no tiny_invariant_1["default"]. Here, the CJS output doesn't use __importDefault "helper" to consume the tiny-invariant package.

I mean, this can be solved by using esModuleInterop: true - but it seems that with moduleResolution: node16... it shouldn't be required.

@alexreardon
Copy link
Owner

Oh damn. So probably best to deprecate 1.3.0 for now until we get a path forward here?

@Andarist
Copy link
Contributor Author

@alexreardon I've just added an edit to my latest comment:

I mean, this can be solved by using esModuleInterop: true - but it seems that with moduleResolution: node16... it shouldn't be required.

So this was always the case, it's not unique for 1.3.0. It's just that perhaps with the package.json#exports+moduleResolution: node16 this shouldn't be mandated.

@alexreardon
Copy link
Owner

I am not totally following.

My most pressing questions:

  1. Should I deprecate 1.3.0?
  2. How should we move forward? Merge this PR and release 1.3.1? Does this PR need to be updated?

@alexreardon
Copy link
Owner

If I can release a fix in 1.3.1 soon, then maybe there is no need to deprecate 🤷‍♂️

@Andarist Andarist force-pushed the fix/ts-module-resolution-node16 branch from 7fe0bb0 to 66eade7 Compare September 28, 2022 09:48
@pkerschbaum
Copy link
Contributor

@alexreardon

  1. Deprecation of 1.3.0: I would say lets try to get a 1.3.1 out soon.
  2. Does this PR need to be updated? yes

@pkerschbaum
Copy link
Contributor

@Andarist here is a repo: https://github.com/pkerschbaum/node-3rxg4d
It is an ESM node16 package importing tiny-invariant. Compile it with npm run build. If you apply your changes directly to tiny-invariant, compilation will break.

@alexreardon
Copy link
Owner

Okay, i'll hold off deprecating 1.3.0. I am happy to merge this PR once @Andarist and @pkerschbaum are happy with it

@Andarist
Copy link
Contributor Author

Ok, I think I somewhat get the problem here - this is madness. It even goes against some official recommendations, such as "put exports.types first". The source of the problem, of course, is the default export and node.js going bonkers with it.

I will apply your suggested changes in a second.

Co-authored-by: Patrick Kerschbaum <patrick.kerschbaum@gmail.com>
@pkerschbaum
Copy link
Contributor

I think the suggestion "put exports.types first" of the TypeScript team means to put it first if you need to tweak it, and it is first in the nested export condition in default. So I think it does not violate that recommendation.
Still, I agree that the whole CJS/ESM TypeScript packages thing is madness 😅

@Andarist
Copy link
Contributor Author

Ok, I've pushed out the change - it's ready to land. cc @alexreardon

@pkerschbaum did you have a chance to take a look at this? What's your take? should the exports.default.types change from export default to export =?

@pkerschbaum
Copy link
Contributor

LGTM now

Actually, the "default" export condition now mirrors "main" and "types" of the package.json - as it should have been from the beginning.

@alexreardon alexreardon merged commit b32b297 into alexreardon:master Sep 28, 2022
@alexreardon
Copy link
Owner

You are both heros

@alexreardon
Copy link
Owner

tiny-invariant@1.3.1-beta.1 is out. @pkerschbaum could you please validate it is now working in your setup?

@alexreardon
Copy link
Owner

alexreardon commented Sep 28, 2022

I have a project setup with the following tsconfig.json that is working all good with tiny-invariant@1.3.1-beta.1

{
  "compilerOptions": {
    "target": "es2020",                                  
    "module": "es2022",                                
    "moduleResolution": "NodeNext"
    }
}

@alexreardon
Copy link
Owner

alexreardon commented Sep 28, 2022

Ok, I've pushed out the change - it's ready to land. cc @alexreardon

@pkerschbaum did you have a chance to take a look at this? What's your take? should the exports.default.types change from export default to export =?

Should we fix that in another PR? Given there is no regression on that one, we could look at a future change to improve it

@Andarist Andarist deleted the fix/ts-module-resolution-node16 branch September 28, 2022 11:14
@pkerschbaum
Copy link
Contributor

v1.3.1 is working from my point of view.

@pkerschbaum
Copy link
Contributor

@Andarist Referring to your comment above - I reproduced the issue here: https://stackblitz.com/edit/node-v9cajx

❯ npm run build-and-run
$ tsc && node ./dist/test.js
TypeError: (0 , tiny_invariant_1.default) is not a function
    at Object.eval (/home/projects/node-v9cajx/dist/test.js:4:30)
    at Object.function (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:15:141393)
    at Module._compile (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:6:219079)
    at Module._extensions..js (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:6:219743)
    at Module.load (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:6:217769)
    at Module._load (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:6:215340)
    at Function.executeUserEntryPoint [as runMain] (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:6:989710)
    at 𝐰𝐜_0x4dc6 (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:6:1603835)
    at _0x34b558 (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:8:36028)
    at _0x5d6daf (https://node-v9cajx.w.staticblitz.com/blitz.b2ae50562a0bdf31ecd871b5c7a8c6dd539613de.js:15:105049)

As you said, in a

  • CJS project
  • with "moduleResolution" set to "node16" and "esModuleInterop" set to false

...the runtime error occurs.

But since no one reported an issue for that, I think there is no need for us to change anything.
"esModuleInterop" should be set to true anyways, as the tsconfig reference suggests:

2022-09-28 22_28_40-TypeScript_ TSConfig Reference - Docs on every TSConfig option - Brave

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.

None yet

3 participants