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

Typescript [4.8.2] is adding invalid javascript for *.cjs files #50647

Closed
dzearing opened this issue Sep 6, 2022 · 22 comments · Fixed by #57896 or #58825
Closed

Typescript [4.8.2] is adding invalid javascript for *.cjs files #50647

dzearing opened this issue Sep 6, 2022 · 22 comments · Fixed by #57896 or #58825
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@dzearing
Copy link
Member

dzearing commented Sep 6, 2022

Bug Report

My project is set to emit ESNext modules. I have a variety of TS files, but I also have a single .cjs file, which needs to remain CommonJS to try and require legacy packages.

My input looks like this:

module.exports = {
  tryRequire(modulePath) {
    return require(modulePath);
  },
};

I expect the output I get from TypeScript 4.7.4:

module.exports = {
  tryRequire(modulePath) {
    return require(modulePath);
  },
};

When I upgrade to TypeScript 4.8.2, it now emits an extra export {} at the end of the .cjs file:

module.exports = {
  tryRequire(modulePath) {
    return require(modulePath);
  },
};
export {}

This causes Node to fail parsing the .cjs file because export does not exist in the .cjs context.

🕗 Version & Regression Information

Worked in 4.7.4, broken in 4.8.2, also broken in current nightly (4.9.0-dev.20220905).

Repro here

https://github.com/dzearing/ts-repro-cjs

@andrewbranch
Copy link
Member

--module esnext combined with --moduleResolution nodenext is not super supported... at the very least, it’s playing the game on hard mode. I’m not quite sure why it “worked” in 4.7 or what we should actually be doing in this case, but I’m assuming it works with --module nodenext? That’s the only mode where we actually support emitting some files as ESM and some as CJS.

@dzearing
Copy link
Member Author

dzearing commented Sep 6, 2022

Yes! That fixed it. I changed to module: "nodenext" and added type: "module" to the package.json to achieve the expected output (ESM by default, CJS for cjs files.)

@dzearing
Copy link
Member Author

dzearing commented Sep 6, 2022

I am unblocked here. I do still think it's not ideal to be emitting invalid cjs but at least I have a workaround. :)

@andrewbranch
Copy link
Member

Agreed. Also related to #48854. @weswigham thoughts?

@igordanchenko
Copy link

igordanchenko commented Sep 13, 2022

I ran into this issue with exactly the same scenario - ESM module with a single .cjs file. But in my case, my tsconfig didn't have any value set for "module" at all, and TS 4.7.4 was totally happy with whatever it was using by default. So it was really unexpected to see the invalid javascript emitted by TS 4.8.2

https://github.com/igordanchenko/sandbox/tree/typescript-50647/main

@igordanchenko
Copy link

igordanchenko commented Sep 13, 2022

Well, I spoke too soon. Setting "module" to "nodenext" did not resolve my issue. It did fix the .cjs file, but now all .ts files are emitted as CommonJS (event though the package.json "type" is set to "module"). I tried setting "moduleResolution" to "nodenext", but tsc compilation completely fails in this case.

index.ts:3:43 - error TS2349: This expression is not callable.
  Type 'typeof import(".../node_modules/clsx/clsx")' has no call signatures.

Any advise will be appreciated.

Here is a minimal repro - https://github.com/igordanchenko/sandbox/tree/typescript-50647/nodenext

@pleerock
Copy link

Have the same issue. Worked on 4.7.4, doesn't work now on 4.8.4 / 4.9.x-rc. Combination of module: "nodenext" and type: "module" doesn't work - compiler starts to complain regarding to import.meta usages in ESM files.

In my use case I need to have a index.cjs (in commonjs) and everything else in ESM (due to commonjs entrypoint requirements in Electron). But now, when index.cjs adds export {} it doesn't work.

@sondreb
Copy link

sondreb commented Nov 24, 2022

But now, when index.cjs adds export {} it doesn't work.

Got the same problem, anyone knows how to avoid TypeScript appending the empty export to .csj files? Does it still on version 4.9.3. It's just the last little issue allowing me to easily mix ESM and a single CommonJS file (auto-generated) in same project. I'll fix it with a manual copy or replace, but would be nice to avoid that.

@reyawn
Copy link

reyawn commented Nov 25, 2022

@sondreb I'm investigating issues with this now in version 4.9.3. Bumping from 4.7.x to 4.9.x indeed broke the compiled output by injecting export {}; into a .cjs file (generated GRPC output).

@unional
Copy link
Contributor

unional commented Jan 3, 2023

@pleerock

In my use case I need to have a index.cjs (in commonjs) and everything else in ESM (due to commonjs entrypoint requirements in Electron). But now, when index.cjs adds export {} it doesn't work.

If it expects cjs, you can't reference ESM code under it. CJS can only import ESM using dynamic import.

You have to compile to CJS. You can't use type: module.

@slimshreydy
Copy link

Any updates on this? Experiencing the same issue where an empty export {} gets appended to the outputted js when compile a .cts file. My package.json has type: module set, and my tsconfig.json has module: esnext and moduleResolution: node. Like @igordanchenko, my tsc fails to compile with the exact same error when switching to module: nodenext.

@andrewbranch
Copy link
Member

.cts file extensions in module: esnext is a contradiction, and there are no plans to make it meaningful at the moment—if anything, it should probably be a program-level error. If you’re running in Node, you need to use --module nodenext and fix the errors. The errors aren’t bugs, they’re real issues that need to be addressed if you expect your code to run safely in Node. For example, this:

index.ts:3:43 - error TS2349: This expression is not callable.
  Type 'typeof import(".../node_modules/clsx/clsx")' has no call signatures.

probably means that you need to add a .default on the end of your dynamic import of a CJS module, because that’s what’s actually required in Node. If you run the JS files produced by tsc in Node, you must use node16 or nodenext for module and moduleResolution. Anything else is 100% incorrect and you will experience problems.

@slimshreydy
Copy link

Ah thank you! You're right; turns out I needed to import clsx as:

import { clsx } from "clsx";

My ts build now works with module: nodenext, and the empty export is gone.

@igordanchenko
Copy link

igordanchenko commented Jan 11, 2023

@slimshreydy, nice catch! Importing the named export indeed solves clsx issue.

@andrewbranch, do you have any thoughts on why importing the default clsx export isn't working under "moduleResolution": "NodeNext"?

import clsx from "clsx";
src/index.tsx:5:21 - error TS2349: This expression is not callable.
  Type 'typeof import("[REDACTED]/node_modules/clsx/clsx")' has no call signatures.

5     <div className={clsx("fancy", "div")}>{children}</div>;
                      ~~~~


Found 1 error in src/index.tsx:5

Here is a minimal repro - https://github.com/igordanchenko/sandbox/tree/typescript-50647/nodenext-1

@andrewbranch
Copy link
Member

Because its typings are wrong. clsx is a CJS library, but its typings say export default. https://unpkg.com/browse/[email protected]/clsx.d.ts

@andrewbranch
Copy link
Member

@weswigham honestly, I think we should consider making export default in CJS-scoped declaration files an error under node16/nodenext, with a message that says the library is probably wrong, similar to #52173. That’s a super common mistake that can often be overlooked in --moduleResolution node.

@joshjg
Copy link

joshjg commented Jul 18, 2023

FYI, the "module": "nodenext" solution does not work if you also need "moduleResolution": "bundler". In my case I'm using bundler because it matches the behavior of my node loader (extensionless and subpath exports).

Option 'bundler' can only be used when 'module' is set to 'es2015' or later.

@knightedcodemonkey
Copy link

knightedcodemonkey commented Jul 29, 2023

The compiler is busted in its handling of the new file extension .mts in combination with --module commonjs. You can see my extended rant in the duplicate issue #51990.

@andrewbranch
Copy link
Member

This should have been closed by #54567

@andrewbranch
Copy link
Member

Well, maybe not until --module commonjs and --module esnext report an error on a file in a conflicting format.

@andrewbranch andrewbranch reopened this Nov 20, 2023
@PythonCoderAS
Copy link

I just ran into this problem with TS 5.3:

Input:

// eslint-disable-next-line import/no-extraneous-dependencies -- This isn't included in the bundle
import { Configuration } from "webpack";

const { resolve } = require("path");

const config: Configuration = {
  entry: "./src/index.ts",
  module: {
    rules: [
      {
        test: /\.tsx?$/,
        use: "ts-loader",
        exclude: /node_modules|\.d\.ts$/,
      },
    ],
  },
  resolve: {
    extensions: [".tsx", ".ts", ".js"],
  },
  output: {
    filename: "bundle.js",
    path: resolve(__dirname, "dist"),
    library: "bbcode_ast",
  },
  devtool: "source-map",
};

module.exports = config;

Output:

const { resolve } = require("path");
const config = {
    entry: "./src/index.ts",
    module: {
        rules: [
            {
                test: /\.tsx?$/,
                use: "ts-loader",
                exclude: /node_modules|\.d\.ts$/,
            },
        ],
    },
    resolve: {
        extensions: [".tsx", ".ts", ".js"],
    },
    output: {
        filename: "bundle.js",
        path: resolve(__dirname, "dist"),
        library: "bbcode_ast",
    },
    devtool: "source-map",
};
module.exports = config;
export {};
//# sourceMappingURL=webpack.config.cjs.map

@andrewbranch
Copy link
Member

This was backed out of 5.5—hoping to reintroduce a fix in 5.6 via #58825.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment