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

Problems with __esModule marker in JS/TS mixed node16 environemnt #58127

Closed
meros opened this issue Apr 9, 2024 · 3 comments · Fixed by #58169
Closed

Problems with __esModule marker in JS/TS mixed node16 environemnt #58127

meros opened this issue Apr 9, 2024 · 3 comments · Fixed by #58169
Assignees
Labels
Bug A bug in TypeScript Domain: JS Emit The issue relates to the emission of JavaScript Fix Available A PR has been opened for this issue

Comments

@meros
Copy link

meros commented Apr 9, 2024

🔎 Search Terms

__esModule, node16, module resolution

🕗 Version & Regression Information

This caused a crash when I assumed synthetic default import would work on a cjs js file, but the resulting output from the cjs js file got the __esModule marker stopping the synthetic default import mechanism. The bug is that the file is certainly not a transpiled es module.

I could not make a convincing repro in playground, so I made a full repo instead:
https://github.com/meros/tsc-module-bug-repro

⏯ Playground Link

https://github.com/meros/tsc-module-bug-repro

💻 Code

In a node16 project, using a mix of js/ts files. CommonJS JS source files get the __esModule marker causing synthetic default imports to fail:

import syntheticDefaultImport from "./cjs-to-cjs-js.cjs";
import * as starImport from "./cjs-to-cjs-js.cjs";

// Will be undefined
console.log("syntheticDefaultImport", syntheticDefaultImport);

// Will be OK
console.log("starImport", starImport);

🙁 Actual behavior

synthetic default import fails due to misleading __esModule marker in output file

🙂 Expected behavior

a JS source that should be treated as, or is detected to be, a commonjs that is transpiled to a commonjs module should NOT get the __esModule marker.

Additional information about the issue

This bug makes the transition to modules in node projects harder and more error prone. I spent at least a day trying to figure out if I misconfigured something or misunderstood the module system but I cannot see how this is not a bug in tsc related to node16 handling.

@fatcerberus
Copy link

If your .cts files contain text like

import syntheticDefaultImport from "./cjs-to-cjs-js.cjs";
import * as starImport from "./cjs-to-cjs-js.cjs";

instead of require() calls, then the resulting .cjs file absolutely is a transpiled ES module.

@meros
Copy link
Author

meros commented Apr 9, 2024

I fully understand that, but the problem is not there. It's in the cjs-to-cjs-js.cjs file:

exports.message = "This is a CommonJS module transpiled to CommonJS (expect no marker).";

This is transpiled to:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.message = "This is a CommonJS module transpiled to CommonJS (expect no marker).";

Note the incorrect Object.defineProperty(exports, "__esModule", { value: true }); line. This causes the TypeScript syntheticDefault function to fail (as it should if this were really a transpiled ESM).

Here is the function that is generated by default import calls translated to require:

var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};

The problem seems to be in shouldEmitUnderscoreUnderscoreESModule that only checks that the file is external and has no 'exportEquals', which is the TypeScript construct:

export = ...

Note that this construct doesn't work with real CommonJS code (no matter if it's in a TS source or JS source) and thus wrongly emits the marker:

exports = ...
module.exports = ...

A reasonable solution could be to extend the test to other variants of exports =, but I don't know enough about the code to understand how this can be done reliably. Another reasonable variant is to never emit this for js files that are indicated to be commonjs already (based on filename, there is a system for this in place)

@andrewbranch
Copy link
Member

The definite bug is that the checking and the emit are misaligned. The checking assumes a “true“ CJS module while the emit produces a transpiled ESM-transpiled-to-CJS module. And I agree the input JS should be the source of truth here; i.e. it should emit as a true CJS module.

@andrewbranch andrewbranch added Bug A bug in TypeScript Domain: JS Emit The issue relates to the emission of JavaScript labels Apr 9, 2024
@andrewbranch andrewbranch self-assigned this Apr 9, 2024
@andrewbranch andrewbranch added this to the TypeScript 5.5.0 milestone Apr 9, 2024
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: JS Emit The issue relates to the emission of JavaScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants