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

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

Open
kraenhansen opened this issue Jun 8, 2023 · 12 comments
Labels
Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@kraenhansen
Copy link

kraenhansen commented Jun 8, 2023

Bug Report

🔎 Search Terms

CommonJS, allowJS, ESM, CJS, file extensions

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about output "file extensions for CommonJS modules".

⏯ 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
Copy link
Author

kraenhansen commented Jun 8, 2023

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

export function getOutputExtension(fileName: string, options: CompilerOptions): Extension {
return fileExtensionIs(fileName, Extension.Json) ? Extension.Json :
options.jsx === JsxEmit.Preserve && fileExtensionIsOneOf(fileName, [Extension.Jsx, Extension.Tsx]) ? Extension.Jsx :
fileExtensionIsOneOf(fileName, [Extension.Mts, Extension.Mjs]) ? Extension.Mjs :
fileExtensionIsOneOf(fileName, [Extension.Cts, Extension.Cjs]) ? Extension.Cjs :
Extension.Js;
}

@kraenhansen
Copy link
Author

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
Copy link
Member

DanielRosenwasser commented Jun 9, 2023

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
Copy link
Member

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

  • .mts files cannot be loaded into a program with --module commonjs (issue an error), or
  • .mts files in --module commonjs override the module setting and emit with ESM syntax

@kraenhansen
Copy link
Author

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
Copy link
Author

kraenhansen commented Jun 12, 2023

@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
Copy link
Member

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 RyanCavanaugh added the Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases label Jun 12, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jun 12, 2023
@RyanCavanaugh
Copy link
Member

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

@knightedcodemonkey
Copy link

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
Copy link

knightedcodemonkey commented Jul 29, 2023

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
Copy link
Author

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
Copy link

knightedcodemonkey commented Jul 31, 2023

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

Congrats on your newborn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants