`.cts` files are should not share the same top-level scope

[nicolo-ribaudo]

Bug Report

NOTE: There are already multiple (open or closed) issues about this topic. However, this issue is specifically about the behavior in .cts files and not in .ts files.

.ts files disallow using top-level block-scoped variables with the same name because they would conflict when loaded with a <script> tag.

.cts files currently have the same restriction, however:

• This extension is explicitly documented as referring to CommonJS files. CommonJS doesn't share a single top-level scope, because every file is wrapped in a function. This means that top-level variables with the same name will not conflict.
• When using CommonJS files, it's common to use the same variable name when importing the same dependency in multiple files. This error makes it impossible to do so.

For these reasons, I think that this restriction should be removed.

A real word example: I'm updating typescript to 4.7 in the Babel repository, and I hoped to be able to convert our remaining JavaScript files to TypeScript. They are still JS because we needed to use the .cjs extension: https://github.com/babel/babel/tree/main/eslint/babel-eslint-parser/src. However, most of those files use the same variables names to import other files, so I'm unable to do so.

🔎 Search Terms

cts top-level Cannot redeclare block-scoped variable

🕗 Version & Regression Information

• Tested in TS4.7, where .cts has been introduced

⏯ Playground Link

n/a (it requires multiple files)

💻 Code

file1.cts:

const { getVisitorKeys } = require("./ast-info.cjs");

file2.cts:

const { getVisitorKeys } = require("./ast-info.cjs");

🙁 Actual behavior

eslint/babel-eslint-parser/src/worker/maybeParse.cts:3:25 - error TS2451: Cannot redeclare block-scoped variable 'getVisitorKeys'.

3 const { getVisitorKeys } = require("./ast-info.cjs");
          ~~~~~~~~~~~~

  eslint/babel-eslint-parser/src/worker/handle-message.cts:3:25
    3 const { getVisitorKeys } = require("./ast-info.cjs");
              ~~~~~~~~~~~~
    'getTokLabels' was also declared here.

🙂 Expected behavior

No error

[nicolo-ribaudo]

I didn't realize that CTS files are meant to be authored using ESM syntax, and then TS will properly scope them.

https://twitter.com/atcb/status/1528501621025406976

[robpalme]

It sounds like the problem here is that the *.cts file is classified as either script or module based on TypeScript's pre-existing disambiguation logic that is also used for *.js files. Meaning you need at least one import or export keyword to have it treated as a module.

That disambiguation is useful for *.js files. But for *.cts & *.cjs files it seems like we already know the intended target is a Node-flavored CommonJS module. And therefore top-level variables should be scoped to the module.

[robpalme]

I was wrong - the disambiguation is more subtle. The disambiguation logic in *.cjs files is more advanced and detects assignments to module.exports and exports property assignments as enabling module mode. So the incorrect erroring is limited to modules that don't export anything - at least according to static analysis.

// @target: esnext
// @module: nodenext
// @allowJs
// @checkJs

// filename: a.cjs
const same = 1;
//    ^ error TS2451: Cannot redeclare block-scoped variable 'same'.
console.log(same);

// filename: b.cjs
const same = 1;
//    ^ error TS2451: Cannot redeclare block-scoped variable 'same'.
console.log(same);

A (bad) workaround is to add an inert line to each file to trigger this module mode detection, such as:

// Inform TypeScript this file is not a script
("exports" in module) || (module.exports = {});

You have to go pretty far down the rabbit hole to figure this out, so I hope we can get auto-detection for this.

[nicolo-ribaudo]

I'm seeing the error in this file, which does export something 🤔

https://github.com/nicolo-ribaudo/babel/blob/up-ts/eslint/babel-eslint-parser/src/worker/maybeParse.cts

[robpalme]

The rule appears to be:

• in cts you need to have import / export
• in cjs you need assignment to module.exports / exports

[Josh-Cena]

Repro:

// test.cts
const a = 1;

// test2.cts
const a = 1;

It's still warning about redeclaring a. The error is only gone when moduleDetection is force. IMO, it should not error even when moduleDetection is legacy, because this is a new extension and we are free to make new rules.

[fatcerberus]

IIRC the ".cts is always a module" rule only applies when using module: node16 (or nodenext). Same for .mts

[Josh-Cena]

Tested with "module": "Node16" and "module": "NodeNext" on 4.7.1-rc—when moduleDetection is auto, there are still errors with both.

[weswigham]

moduleDetection: force will force module parsing~

[weswigham]

Just a small update: we discussed this a bit today - we'll update auto to always interpret cjs and cts files as modules, but we'll also make module: node16 (and nodenext) imply moduleDetection: force, since in reality nobody actually wants any of their files to be interpreted as scripts when targeting node explicitly. Even though these are technically breaking changes, they probably won't break any real code and we'll probably ship them in the next patch. (Assuming we don't need any API breaks to support them.)