Merge pull request #85 from arethetypeswrong/checker

Improved default export analysis

Author: andrewbranch

.changeset/sweet-mangos-marry.md

@@ -0,0 +1,9 @@
+---
+"@arethetypeswrong/core": minor
+---
+
+New problem kind: **Missing `export =`**
+
+Previously, `FalseExportDefault` had many cases of false positives where the JavaScript assigned an object to `module.exports`, and that object had a `default` property pointing back to itself. This pattern is not a true `FalseExportDefault`, but it is still problematic if the types only declare an `export default`. These kinds of false positives of `FalseExportDefault` are now instead reported as `MissingExportEquals`.
+
+Additionally, `FalseExportDefault` was only ever reported as being visible in `node16-esm`, but this was incorrect. The consequences are most likely to be visible in `node16-esm`, but the problem is fundamentally independent of the module resolution mode, and inaccuracies can be observed in other modes as well, especially when `esModuleInterop` is not enabled.

docs/problems/MissingExportEquals.md

@@ -0,0 +1,77 @@
+# ❓ Missing `export =`
+
+The JavaScript appears to set both `module.exports` and `module.exports.default` for improved compatibility, but the types only reflect the latter (by using `export default`). This will cause TypeScript under the `node16` module mode to think an extra `.default` property access is required, which will work at runtime but is not necessary. These types `export =` an object with a `default` property instead of using `export default`.
+
+## Explanation
+
+This problem occurs when a CommonJS JavaScript file appears to use a compatibility pattern like:
+
+```js
+class Whatever {
+  /* ... */
+}
+Whatever.default = Whatever;
+module.exports = Whatever;
+```
+
+but the corresponding type definitions only reflect the existence of the `module.exports.default` property:
+
+```ts
+declare class Whatever {
+  /* ... */
+}
+export default Whatever;
+```
+
+The types should declare the existence of the `Whatever` class on both `module.exports` and `module.exports.default`. The method of doing this can vary depending on the kinds of things already being exported from the types. When the `export default` exports a class, and that class is the only export in the file, the `default` can be declared as a static property on the class, and the `export default` swapped for `export =`:
+
+```ts
+declare class Whatever {
+  static default: typeof Whatever;
+  /* ... */
+}
+export = Whatever;
+```
+
+When the file exports additional types, it will be necessary to declare a `namespace` that merges with the class and contains the exported types:
+
+```ts
+declare class Whatever {
+  static default: typeof Whatever;
+  /* ... */
+}
+declare namespace Whatever {
+  export interface WhateverProps {
+    /* ... */
+  }
+}
+export = Whatever;
+```
+
+This merging namespace can also be used to declare the `default` property when the main export is a function:
+
+```ts
+declare function Whatever(props: Whatever.WhateverProps): void;
+declare namespace Whatever {
+  declare const _default: typeof Whatever;
+  export { _default as default };
+
+  export interface WhateverProps {
+    /* ... */
+  }
+}
+export = Whatever;
+```
+
+## Consequences
+
+This problem is similar to the [“Incorrect default export”](./FalseExportDefault.md) problem, but in this case, the types are _incomplete_ rather than wholly incorrect. This incompleteness may lead TypeScript users importing from Node.js ESM code, or CommonJS code without `esModuleInterop` enabled, to add an extra `.default` property onto default imports to access the module’s `module.exports.default` property, even though accessing the `module.exports` would have been sufficient.
+
+```ts
+import Whatever from "pkg";
+Whatever.default(); // Ok, but `Whatever()` would have worked!
+```
+
+## Common causes
+
+This problem is usually caused by library authors incorrectly hand-authoring declaration files to match existing JavaScript rather than generating JavaScript and type declarations from TypeScript with `tsc`, or by using a third-party TypeScript emitter that adds an extra compatibility layer to TypeScript written with `export default`. Libraries compiling to CommonJS should generally avoid writing `export default` as input syntax.

packages/cli/src/problemUtils.ts

@@ -11,6 +11,7 @@ export const problemFlags: Record<ProblemKind, string> = {
   FallbackCondition: "fallback-condition",
   CJSOnlyExportsDefault: "cjs-only-exports-default",
   FalseExportDefault: "false-export-default",
+  MissingExportEquals: "missing-export-equals",
   UnexpectedModuleSyntax: "unexpected-module-syntax",
   InternalResolutionError: "internal-resolution-error",
 };

packages/cli/test/snapshots/@[email protected]

@@ -6,14 +6,16 @@ $ attw @[email protected] -f table-flipped
 
 @vitejs/plugin-react v3.1.0
 
+❓ The JavaScript appears to set both module.exports and module.exports.default for improved compatibility, but the types only reflect the latter (by using export default). This will cause TypeScript under the node16 module mode to think an extra .default property access is required, which will work at runtime but is not necessary. These types export = an object with a default property instead of using export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/MissingExportEquals.md
+
 🎭 Import resolved to a CommonJS type declaration file, but an ESM JavaScript file. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md
 
 
-┌────────────────────────┬────────┬───────────────────┬────────────────────────┬─────────┐
-│                        │ node10 │ node16 (from CJS) │ node16 (from ESM)      │ bundler │
-├────────────────────────┼────────┼───────────────────┼────────────────────────┼─────────┤
-│ "@vitejs/plugin-react" │ 🟢     │ 🟢 (CJS)          │ 🎭 Masquerading as CJS │ 🟢      │
-└────────────────────────┴────────┴───────────────────┴────────────────────────┴─────────┘
+┌────────────────────────┬───────────────────────┬───────────────────────┬────────────────────────┬─────────┐
+│                        │ node10                │ node16 (from CJS)     │ node16 (from ESM)      │ bundler │
+├────────────────────────┼───────────────────────┼───────────────────────┼────────────────────────┼─────────┤
+│ "@vitejs/plugin-react" │ ❓ Missing `export =` │ ❓ Missing `export =` │ 🎭 Masquerading as CJS │ 🟢      │
+└────────────────────────┴───────────────────────┴───────────────────────┴────────────────────────┴─────────┘
 
 
 ```

packages/cli/test/snapshots/[email protected]

@@ -11,16 +11,16 @@ Build tools:
 - rollup@^2.44.0
 - @rollup/plugin-typescript@^10.0.1
 
- No problems found 🌟
+❓ The JavaScript appears to set both module.exports and module.exports.default for improved compatibility, but the types only reflect the latter (by using export default). This will cause TypeScript under the node16 module mode to think an extra .default property access is required, which will work at runtime but is not necessary. These types export = an object with a default property instead of using export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/MissingExportEquals.md
 
 
-┌───────┬────────┬───────────────────┬───────────────────┬─────────┐
-│       │ node10 │ node16 (from CJS) │ node16 (from ESM) │ bundler │
-├───────┼────────┼───────────────────┼───────────────────┼─────────┤
-│ "ajv" │ 🟢     │ 🟢 (CJS)          │ 🟢 (CJS)          │ 🟢      │
-└───────┴────────┴───────────────────┴───────────────────┴─────────┘
+┌───────┬───────────────────────┬───────────────────────┬───────────────────────┬───────────────────────┐
+│       │ node10                │ node16 (from CJS)     │ node16 (from ESM)     │ bundler               │
+├───────┼───────────────────────┼───────────────────────┼───────────────────────┼───────────────────────┤
+│ "ajv" │ ❓ Missing `export =` │ ❓ Missing `export =` │ ❓ Missing `export =` │ ❓ Missing `export =` │
+└───────┴───────────────────────┴───────────────────────┴───────────────────────┴───────────────────────┘
 
 
 ```
 
-Exit code: 0
+Exit code: 1

packages/cli/test/snapshots/[email protected]

@@ -10,7 +10,7 @@ Build tools:
 - typescript@^4.8.4
 - rollup@^2.67.0
 
-❓ Wildcard subpaths cannot yet be analyzed by this tool. https://github.com/arethetypeswrong/arethetypeswrong.github.io/issues/40
+🃏 Wildcard subpaths cannot yet be analyzed by this tool. https://github.com/arethetypeswrong/arethetypeswrong.github.io/issues/40
 
 💀 Import failed to resolve to type declarations or JavaScript files. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md
 
@@ -24,7 +24,7 @@ Build tools:
 ├─────────────────────────────────────────┼──────────────────────┼──────────────────────────────┼────────────────────┼────────────────────┤
 │ "axios"                                 │ 🟢                   │ 🟢 (CJS)                     │ 🟢 (ESM)           │ 🟢                 │
 ├─────────────────────────────────────────┼──────────────────────┼──────────────────────────────┼────────────────────┼────────────────────┤
-│ "axios/unsafe/*"                        │ ❓ Unable to check   │ ❓ Unable to check           │ ❓ Unable to check │ ❓ Unable to check │
+│ "axios/unsafe/*"                        │ 🃏 Unable to check   │ 🃏 Unable to check           │ 🃏 Unable to check │ 🃏 Unable to check │
 ├─────────────────────────────────────────┼──────────────────────┼──────────────────────────────┼────────────────────┼────────────────────┤
 │ "axios/unsafe/core/settle.js"           │ 💀 Resolution failed │ ❌ No types                  │ ❌ No types        │ ❌ No types        │
 │                                         │                      │ ⚠️ ESM (dynamic import only) │                    │                    │

packages/cli/test/snapshots/[email protected]

@@ -9,11 +9,11 @@ hexoid v1.0.0
 ❗️ The resolved types use export default where the JavaScript file appears to use module.exports =. This will cause TypeScript under the node16 module mode to think an extra .default property access is required, but that will likely fail at runtime. These types should use export = instead of export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseExportDefault.md
 
 
-┌──────────┬────────┬───────────────────┬──────────────────────────────┬─────────┐
-│          │ node10 │ node16 (from CJS) │ node16 (from ESM)            │ bundler │
-├──────────┼────────┼───────────────────┼──────────────────────────────┼─────────┤
-│ "hexoid" │ 🟢     │ 🟢 (CJS)          │ ❗️ Incorrect default export │ 🟢      │
-└──────────┴────────┴───────────────────┴──────────────────────────────┴─────────┘
+┌──────────┬──────────────────────────────┬──────────────────────────────┬──────────────────────────────┬──────────────────────────────┐
+│          │ node10                       │ node16 (from CJS)            │ node16 (from ESM)            │ bundler                      │
+├──────────┼──────────────────────────────┼──────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
+│ "hexoid" │ ❗️ Incorrect default export │ ❗️ Incorrect default export │ ❗️ Incorrect default export │ ❗️ Incorrect default export │
+└──────────┴──────────────────────────────┴──────────────────────────────┴──────────────────────────────┴──────────────────────────────┘
 
 
 ```