denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.95k stars 5.23k forks source link

npm default export is incorrectly typed for clsx #19096

Open pting-me opened 1 year ago

pting-me commented 1 year ago

Describe the bug

npm:* default exports are incorrectly typed. Packages that are exported as default are assumed to have a default property. The plugin will claim a type error when there are no actual runtime errors. Furthermore, trying to appease the plugin error by using the default property will actually result in runtime errors.

To Reproduce

  1. Create a file script.ts
import clsx from 'npm:clsx';

console.log(clsx("hello", "goodbye"));
  1. Cache dependencies, notice the following error:
This expression is not callable.
  Type 'typeof import("file:///Users/pting/Library/Caches/deno/npm/registry.npmjs.org/clsx/1.2.1/clsx")' has no call signatures.deno-ts(2349)
  1. Try running deno run script.ts, notice there are no errors at runtime

  2. Try appeasing the compiler:

    console.log(clsx.default("hello", "goodbye"));
  3. Notice runtime error.

Expected behavior

Expected plugin parsing behavior to match runtime behavior.

Screenshots

Screen Shot 2023-04-27 at 9 54 50 PM

Versions

vscode: 1.77.3 deno: 1.33.0 extension: 3.17.0

pting-me commented 1 year ago

It looks like the file it's trying to resolve doesn't exist in my file system.

It's trying to resolve {path-to-cache}/deno/npm/registry.npmjs.org/clsx/1.2.1/clsx.

If I explicitly set @deno-types to .../clsx/1.2.1/dist/clsx.js it seems to work fine.

My main point of confusion is why deno itself does not raise any red flags, but it's throwing compiler errors here.

not-my-profile commented 1 year ago

I also just ran into this bug twice with two other packages (postcss-nesting and windicss). So this bug appears to be more general and less specific to clsx.

The following should type check: ```ts import postcssNesting from "npm:postcss-nesting@11.3.0"; postcssNesting(); ``` but `deno check` fails with the error message: ``` error: TS2349 [ERROR]: This expression is not callable. Type 'typeof import("file:///home/martin/.cache/deno/npm/registry.npmjs.org/postcss-nesting/11.3.0/dist/index.d.ts")' has no call signatures. postcssNesting(); ~~~~~~~~~~~~~~ ``` Likewise the following should type check: ```ts import Processor from "npm:windicss@3.5.6"; const processor = new Processor(); ``` but `deno check` fails with the error message: ``` error: TS2351 [ERROR]: This expression is not constructable. Type 'typeof import("file:///home/martin/.cache/deno/npm/registry.npmjs.org/windicss/3.5.6/index.d.ts")' has no construct signatures. const processor = new Processor(); ~~~~~~~~~ ```

My main point of confusion is why deno itself does not raise any red flags, but it's throwing compiler errors here.

Deno is no longer using proper TypeScript but has started using a fork to support npm specifiers, see #16332.

pting-me commented 1 year ago

Yeah I meant to use clsx as an example but I guess the title got renamed.

Deno is no longer using proper TypeScript but has started using a fork to support npm specifiers, see https://github.com/denoland/deno/pull/16332.

That's good to know. I assume that's causing the discrepancy because VSCode will not be able to use that fork, is that correct?

I guess in the end most people are using URL imports nowadays but certain packages (Vite in particular comes to mind) work with the npm:* counterpart but not on esm.sh.

not-my-profile commented 1 year ago

I assume that's causing the discrepancy because VSCode will not be able to use that fork, is that correct?

No that's not correct. You can just install the Deno extension for VSCode, which uses the language server that's part of the deno binary, which in turn uses the forked TypeScript version. (Opening a directory with a deno.json file in VSCode will then prompt you to switch to the Deno language server.)

I guess in the end most people are using URL imports nowadays

Deno 1.28 has stabilized npm compatibility in the sense that you no longer need to use --unstable to use npm: specifiers, which makes the bug described by this issue quite severe in my opinion, since you'd expect a "stabilized" implementation to support a feature as common as default exports.

dsherret commented 1 year ago

This has to do with Deno's Node module resolution implementation for type checking and is not related to the fork (the fork is for multiple globalThis symbols).

In this case, these packages have incorrect types, but TypeScript's Node module resolution can sort it out, but Deno's can't at the moment.

https://arethetypeswrong.github.io/?p=clsx%401.2.1 https://arethetypeswrong.github.io/?p=postcss-nesting%4011.3.0 https://arethetypeswrong.github.io/?p=windicss%403.5.6

not-my-profile commented 1 year ago

Ah, thanks for clarifying that. I am curious: is the node module resolution implemented in Rust? Could you point me to where it's implemented?

dsherret commented 1 year ago

It's called here from TypeScript:

https://github.com/denoland/deno/blob/dd508c9c8970fc6565cfd50cd3e01e8571425347/cli/tsc/99_main_compiler.js#L600-L603

Then rust code starts here (see call to resolve_non_graph_specifier_types):

https://github.com/denoland/deno/blob/dd508c9c8970fc6565cfd50cd3e01e8571425347/cli/tsc/mod.rs#L515

not-my-profile commented 1 year ago

Ah, thanks! Are you sure that this problem only occurs with packages that have incorrect types? If so what's wrong with the @denotest/types-export-default module of #19669?

dsherret commented 1 year ago

@not-my-profile it's a cjs dts file that has a default export.