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

import { default as ... } ... doesn't work as of 4.8.3 #50690

Closed
alecmev opened this issue Sep 8, 2022 · 18 comments
Closed

import { default as ... } ... doesn't work as of 4.8.3 #50690

alecmev opened this issue Sep 8, 2022 · 18 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@alecmev
Copy link

alecmev commented Sep 8, 2022

Bug Report

🔎 Search Terms

export default, import default as, namespace, declaration.

🕗 Version & Regression Information

This changed between versions 4.8.2 and 4.8.3.

⏯ Playground Link

I can't provide a link because both Bug Workbench and CodeSandbox still don't have 4.8.3.

💻 Code

// @flatten-js/core/index.d.ts
declare namespace Flatten {
    class Polygon {
        constructor();
    }
}

export default Flatten;

Complete typings.

// index.ts
import { default as Flatten } from '@flatten-js/core';

const x = new Flatten.Polygon();

🙁 Actual behavior

I get the following error:

error TS2339: Property 'Polygon' does not exist on type ...

When looking at Flatten, it still has a default within, so I need to access Polygon with Flatten.default.Polygon.

🙂 Expected behavior

It gets unwrapped correctly, like in 4.8.2 and before. Right now I'm having to use the following workaround:

import { default as FlattenBad } from '@flatten-js/core';
const Flatten = FlattenBad as unknown as typeof FlattenBad.default;
@andrewbranch
Copy link
Member

TL;DR: the library’s types are wrong.

@flatten-js/core is a CommonJS module (it ships some ESM files, but these are not exposed through any standard package.json fields). This means when you do a default import of it (or a { default as Whatever } import) in Node, you get the whole module.exports object. @flatten-js/core’s module.exports is an object with roughly the same shape as Flatten, including a member named default whose value is Flatten.

❯ node --input-type=module -e 'import Flatten from "@flatten-js/core"; console.log(Flatten === Flatten.default)'
false

❯ node --input-type=module -e 'import Flatten from "@flatten-js/core"; console.log(Flatten.box === Flatten.default.box)'
true

The way you represent this in a .d.ts file is something like this:

declare function box(): any;
declare const Flatten: {
  box: typeof box;
}

export { box };
export { Flatten as default };

But instead, as you noted, the .d.ts file included says export default Flatten. This is not the same thing. It is, admittedly, really puzzling to try to figure out what export default means when you know from the file extension (.d.ts) and package.json type (omitted entirely) that you are looking at a CommonJS module, not an ES module. But we do our best and say that it represents something like module.exports.default = Flatten. Which is something that the JS file actually has, which is great, but it’s also woefully incomplete. It’s missing every other export besides default, which is why, when you import it, TypeScript shows you that it has a default and nothing else. That’s exactly what the typings say.

Now, you may object, saying that obviously we should just resolve to the default member of the module record here. But that would be incorrect. Consider this perfectly correct JS/.d.ts pair:

// index.js
Object.defineProperty(exports, '__esModule', { value: true });
module.exports.foo = "foo";
module.exports.default = "default";

// index.d.ts
export declare const foo: "foo";
declare const _default: "default";
export default _default;

What should we say the type of lib is when you import it?

import lib from "my-wacky-lib";

By your hypothetical objection, it would look like lib should be "default". But in fact, in Node, it’s an object containing properties foo and default. This is different from what runtimes/transpilers/bundlers who care about that __esModule flag would give you, so it’s quite important we get this right and warn you that Node is going to return the whole module record.

The author of @flatten-js/core was clever to make module.exports virtually indistinguishable from module.exports.default—it means that consumers will see the same API regardless of whether their module resolver cares about __esModule. Unfortunately, that same clever duplication was not copied into the type declaration file.

The fact that { default as Flatten } worked was a bug, and was causing problems for libraries that are typed correctly: #49567.

@andrewbranch
Copy link
Member

@weswigham, please double check my explanation 😅

@andrewbranch andrewbranch added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 9, 2022
@dkulchenko
Copy link

dkulchenko commented Sep 9, 2022

Chiming in here - while I understand that the old behavior working correctly may have been unintentional, this change in 4.8.3 led to a bunch of breaking changes in my code, which doesn't seem right for a point release.

The { default as X } clause is useful in working around issues with types when you have an ESM nodenext app and import a RequireJS dependency with a default export whose types haven't yet been updated for nodenext support as suggested in #48845.

For example, see the ioredis ESM import snippet (which was correct as of 4.8.2 but no longer works): https://github.com/luin/ioredis#basic-usage

This worked in 4.8.2:

import { default as Redis } from "ioredis";

const redisConn: Redis = new Redis();

but is broken in 4.8.3, needing to be rewritten as:

import { default as Redis } from "ioredis";

const redisConn: Redis.Redis = new Redis.default();

to compile. I had to make a bunch of similar changes to fastify plugin imports as well. It looks like ajv is affected as well (ajv-validator/ajv#2047), and likely many other packages.

This seems like a pretty major non-backwards-compatible change for a point release.

@alecmev
Copy link
Author

alecmev commented Sep 9, 2022

Thank you for such a thorough explanation, @andrewbranch! I'll report this in Flatten instead.

@andrewbranch
Copy link
Member

@dkulchenko yeah, I understand. We didn’t realize that the bug was being used as a workaround for broken typings. Knowing what I know now, I probably would have held the fix from 4.8.3, but the proverbial toothpaste is out of the tube now.

One correction, though—the typings that are broken were always wrong; they didn’t just need to be updated for nodenext. The problem is just more observable in nodenext. Hopefully the problem libraries will get updated quickly.

@fatcerberus
Copy link

the proverbial toothpaste is out of the tube now

Haven't heard this one before! It just sounds so... prosaic, compared to more colorful expressions like the cat being out of the bag, or the genie being out of the bottle, or closing the barn door after the horse escaped...

@meteorlxy
Copy link

meteorlxy commented Sep 16, 2022

needing to be rewritten as:

import { default as Redis } from "ioredis";

const redisConn: Redis.Redis = new Redis.default();

The workaround becomes even stranger than before 😅

Update: the workaround seems only works for type checking, but the compiled result would failed to be used in other tools like Vite

@pastelmind
Copy link

pastelmind commented Sep 20, 2022

I'm stuck on a similar situation. I am writing ESM code with TypeScript 4.8.3 and moduleResolution: "Node16". I import a CommonJS library whose code looks like:

// node_modules/sql.js/dist/sql-wasm-debug.js, lines 7061-7063
function initSqlJs() { /* ... */ };
module.exports = initSqlJs;
module.exports.default = initSqlJs;

and whose type definitions look like:

// node_modules/@types/sql.js/index.d.ts
export interface InitSqlJsStatic extends Function {
    (config?: SqlJsConfig): Promise<SqlJsStatic>;
    readonly default: this;
}

declare global {
    let SqlJs: InitSqlJsStatic;
}

export default SqlJs;

...and when I attempt to call someFunc() like this, I run into type errors:

import initSqlJs from 'sql.js';

// error TS2349: This expression is not callable.
//  Type 'typeof import("/path/to/my/project/node_modules/@types/sql.js/index")' has no call signatures.
initSqlJs();

...even though the code runs fine in Node.js.

If the fault lies with the type definitions for sql.js, how can the type definitions be corrected without altering the JS source itself?

@andrewbranch
Copy link
Member

It seems like they’re trying to do a UMD module

interface InitSqlJsStatic extends Function {
    (config?: SqlJsConfig): Promise<SqlJsStatic>;
    readonly default: this;
}

declare const SqlJs: InitSqlJsStatic;
declare namespace SqlJs {
    export { InitSqlJsStatic };
}

export = SqlJs;
export as namespace SqlJs;

pastelmind pushed a commit to pastelmind/DefinitelyTyped that referenced this issue Sep 20, 2022
Rewrite the export statements so that the package can be used correctly
in projects using `module: "Node16"` as well as browser scripts.

Previously, the type definitions for sql.js incorrectly modeled how the
library actually behaves. The typings used `export default` even though
sql.js is actually a UMD library. This works fine in projects with
`module: "ESNext"`, but caused errors when using `module: "Node16"` due
to a recent bugfix in TypeScript 4.8.3+. For details, see
microsoft/TypeScript#50690 .

Also, the type definitions previously used 'SqlJs' as the global
namespace for browser script contexts. However, the library actually
uses `initSqlJs` when loaded with the <script> tag in browsers. This
caused compiler errors when using `/// <reference types="sql.js" />`.

These issues have been fixed by using `export =`, `import x = require()`
and `export as namespace` statements with the correct identifier.
typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Sep 24, 2022
…nvironments by @pastelmind

Rewrite the export statements so that the package can be used correctly
in projects using `module: "Node16"` as well as browser scripts.

Previously, the type definitions for sql.js incorrectly modeled how the
library actually behaves. The typings used `export default` even though
sql.js is actually a UMD library. This works fine in projects with
`module: "ESNext"`, but caused errors when using `module: "Node16"` due
to a recent bugfix in TypeScript 4.8.3+. For details, see
microsoft/TypeScript#50690 .

Also, the type definitions previously used 'SqlJs' as the global
namespace for browser script contexts. However, the library actually
uses `initSqlJs` when loaded with the <script> tag in browsers. This
caused compiler errors when using `/// <reference types="sql.js" />`.

These issues have been fixed by using `export =`, `import x = require()`
and `export as namespace` statements with the correct identifier.

Co-authored-by: Yehyoung Kang <[email protected]>
pkerschbaum added a commit to pkerschbaum/pkerschbaum-homepage that referenced this issue Sep 28, 2022
pkerschbaum added a commit to pkerschbaum/pkerschbaum-homepage that referenced this issue Sep 28, 2022
@Mihai-github
Copy link

Hi, I'm having the same issue using "typescript": "^4.8.2" with React Native. There's any recommended suggestion on how to fix this or should I downgrade the version :(?

@andrewbranch
Copy link
Member

@Mihai-github can you give us some more details on what you’re doing (including tsconfig settings) and what you’re seeing?

@Mihai-github
Copy link

Mihai-github commented Sep 30, 2022

@adamburgess Sure, thanks.
My tsconfig.json:

{
    "compilerOptions": {
        "allowJs": true,
        "allowSyntheticDefaultImports": true,
        "esModuleInterop": false,
        "isolatedModules": true,
        "jsx": "preserve",
        "lib": ["es6", "esnext"],
        "moduleResolution": "node",
        "resolveJsonModule": true,
        "noEmit": true,
        "target": "es6",
        "module": "esnext",
        "skipLibCheck": true,
        "forceConsistentCasingInFileNames": true,
        "baseUrl": "src"
    },
    "include": ["src/**/*", "src", "index.d.ts"],
    "exclude": [
        "node_modules",
        "babel.config.js",
        "metro.config.js",
        "jest.config.js"
    ]
}

My issue is probably the same with others with export {default as XYZ} from './abcd'

When I'm trying to import the component in other files using the name given with this current version of typescript I cannot make it work because I get an error telling Cannot read properties of undefined (reading 'XYZ').

@andrewbranch
Copy link
Member

What import/re-export is giving you the problem?

@GabenGar
Copy link

GabenGar commented Oct 6, 2022

How do I fix this issue for a ESM-only javascript package the types for which are provided by DefinitelyTyped?
The package: https://github.com/yeoman/stringify-object
The types: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/stringify-object

@andrewbranch
Copy link
Member

@GabenGar a package.json with "type": "module" needs to be added to the DefinitelyTyped package.

@nickluger
Copy link

but is broken in 4.8.3, needing to be rewritten as:

import { default as Redis } from "ioredis";

const redisConn: Redis.Redis = new Redis.default();

This works for me... 🤷‍♂️

import RedisModule from "ioredis";

const Redis = RedisModule.default;

@Mister-Hope
Copy link

Mister-Hope commented Nov 19, 2022

Hi there, this issue is heavily effecting my projects.

I am targeting pure esm packages with target: es2020 (compatible with node14) and moduleResolution: NodeNext in my project.

There is a lot of packages in my repo with is exporting both esm files with exports and module and cjs files with main, but not having type: module or even use type: commonjs

I can only managed to fix types issues with them by adding type: module. My concern is:

  • Is this solution correct? Does this have side effects?
  • Is this internal changes being so breaking?

With just one hour, I am already sending severl prs to upsteam. including:

The examples above are popular packages,(e.g.: mitt is getting Weekly Downloads at 1,960,326). If the whole ecosystem has a huge part that needs to change according to this, I am thinking that the change might be too breaking for a minor version.

@andrewbranch Could you give some feedback with this?

@Mister-Hope
Copy link

I read again https://nodejs.org/dist/latest-v18.x/docs/api/packages.html#exports, it seems that my solution adding type: module may break cjs requires, so how shall I solve this? A .d.mts declaration file or what?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

10 participants