microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.2k stars 12.51k forks source link

Using `"allowJs": true` and `"module": "commonjs"` to transform `.mjs` files should emit `.cjs` files #54573

Open kraenhansen opened 1 year ago

kraenhansen commented 1 year ago

Bug Report

πŸ”Ž Search Terms

CommonJS, allowJS, ESM, CJS, file extensions

πŸ•— Version & Regression Information

⏯ Playground Link

Sandbox link with relevant code (I couldn't use the playground, since this involves using an .mjs file).

πŸ’» Code

// package.json
{
  "name": "tsc-mjs-extension-bug",
  "version": "0.1.0",
  "type": "commonjs",
  "main": "dist/javascript.mjs",
  "scripts": {
    "build": "tsc",
    "test": "node ."
  },
  "dependencies": {
    "typescript": "5.1.3"
  }
}
// tsconfig.json
{
  "compilerOptions": {
    "module": "commonjs",
    "moduleResolution": "nodenext",
    "allowJs": true,
    "outDir": "dist"
  }
  "include": ["src"]
}
// src/javascript.mjs
export const where = "javascript";
console.log(`Hello ${where}`);
// src/typescript.ts
export const where = "typescript";
console.log(`Hello ${where}`);

πŸ™ Actual behavior

The javascript.mjs is transformed to CommonJS (as expected), but the file extension of the emitted file is still .mjs. This breaks the package, since .mjs is supposed to be use exclusively for JavaScript using ESM and the emitted file now uses CommonJS.

πŸ™‚ Expected behavior

I would expect tsc to transform the src/javascript.mjs to CommonJS and either:

  1. emit it as dist/javascript.cjs or alternatively
  2. take into account the "type": "commonjs" in the package.json and emit the file as .js as it does with the src/typescript.ts file.
kraenhansen commented 1 year ago

I did a bit of digging and I believe this is the piece of code responsible for determining the extension of the output file:

https://github.com/microsoft/TypeScript/blob/e49a15f6339381213d7785bc3c78e5bb275a3206/src/compiler/emitter.ts#L574-L580

kraenhansen commented 1 year ago

Related, I found these minutes from a design meeting on the feature to support .mjs and .cjs as input files. Also this issue seem tangental, although I understand it more about the file extension for .ts source files.

DanielRosenwasser commented 1 year ago

I think that #35148 (and #35589) is related; however, I don't recall whether that also did path rewriting - and we didn't have the context of #44442 at the time.

As you pointed out, in #44442 we discussed the issue of .mjs as an input file under "module": "commonjs" being a weird "undefined behavior" of the compiler. I'm still not sure if the right thing to do here is to convert to .cjs in the output. I could kind of see that, but I don't really know if we have the appetite for a new compiler option to solve this case (as in your PR at #54583).

Is there any reason you don't want to just use .js as an input file extension?

andrewbranch commented 1 year ago

I agree the behavior is bad, but I think the solution we want is either

kraenhansen commented 1 year ago

Is there any reason you don't want to just use .js as an input file extension?

Yes. Since the package has "type": "commonjs" it would be wrong for the ESM source-file to have a .js extension, which could trip up static analysis tools and linters relying on the semantics of these extensions in relation to the package.json's type.

kraenhansen commented 1 year ago

@andrewbranch If using .mts instead of .ts is mainly a way to control the output file extension, I can see why that would make sense and would prefer an error instead of an implicit setting override.

I assume both suggestions would also apply to .mjs files and I'd expect that to remove the ability to transpile .mjs to commonjs? I think that's a powerful feature, which would be great the the compiler would continue to support, although admittedly on the edge of use-cases that are expected from a TypeScript compiler.

RyanCavanaugh commented 1 year ago

If the code is intended to always target CJS, it should just be a cts file.

A naive syntactic transform from ESM to CJS is liable to end up importing semantically different modules from when the code was written, causing unpredictable breaks. This isn't something that you can "just" do in the general case.

RyanCavanaugh commented 1 year ago

Tagging as backlog for either of the proposed behaviors, not the behavior described in the OP

knightedcodemonkey commented 1 year ago

Converting .mts files to anything other than ESM is bad and breaks things. Specifying an .mts extension should indicate that this files module system is fixed as ESM without exception and regardless of the --module setting.

This is the correct approach.

.mts files in --module commonjs override the module setting and emit with ESM syntax

Please fix this.

knightedcodemonkey commented 1 year ago

The javascript.mjs is transformed to CommonJS (as expected)

Why would that be expected? If you specify a file as .mjs then it should always be an ES module. tsc converting that to CJS is just wrong.

.mts files cannot be loaded into a program with --module commonjs (issue an error)

Yes, a compiler error, not user error.

Any .mts, or .cts (same for the JS variants) should not have its module system changed. Ever.

Also, why was this labelled Possible Improvement and not a bug?

kraenhansen commented 1 year ago

The javascript.mjs is transformed to CommonJS (as expected)

Why would that be expected?

Because the tsconfig has module set to "commonjs" in the example I provided, I'd expect all files emitted to be using that, regardless of the module system used by the input file.

knightedcodemonkey commented 1 year ago

This is where you and TypeScript differ with the way the Node.js runtime operates against file extensions. Too bad.

Congrats on your newborn.