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

CommonJS module output broken after updating from 5.5.4 to 5.6.2 #59991

Closed
SBoudrias opened this issue Sep 17, 2024 · 8 comments
Closed

CommonJS module output broken after updating from 5.5.4 to 5.6.2 #59991

SBoudrias opened this issue Sep 17, 2024 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@SBoudrias
Copy link

SBoudrias commented Sep 17, 2024

🔎 Search Terms

5.6 commonjs cjs node10 moduleResolution modules regression

🕗 Version & Regression Information

⏯ Playground Link

SBoudrias/Inquirer.js#1554

💻 Code

My tsconfig.cjs.json

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "lib": ["es2023"],
    "target": "es6",
    "module": "commonjs",
    "moduleResolution": "node10",
    "outDir": "dist/cjs",
    "declarationDir": "dist/cjs/types"
  }
}

🙁 Actual behavior

The files outputted in dist/cjs are now using esm module syntax instead fo cjs (import ... from).

🙂 Expected behavior

As of version 5.5.4, the files outputted in dist/cjs were cjs (require(...)).

Additional information about the issue

No response

@Andarist
Copy link
Contributor

bisects to #59479

@Andarist
Copy link
Contributor

So it seems that before this PR it would ignore the fact that your files are authored using .mts and it would allow you to compile that to a commonjs module. With the new version the extension is always taken into consideration, even with "moduleResolution": "node10".

We can trace back this work to #57896 and #58825 . They mention that from now on extensions should be considered in more module modes but they are not as explicit when it comes to what does it mean for moduleResolution modes.

This is a one-line change to fix your issue but the changed code is also almost explicit when it comes to applying the new logic for extensions regardless of moduleResolution... so I think it's best to ping @andrewbranch and defer to his opinion about this.

Minimal repro case:

// @moduleResolution: node10
// @module: commonjs

// @filename: index.mts
export const foo = ''

@andrewbranch
Copy link
Member

Yes, this is working as intended. Compiling .mts files to CommonJS-containing .mjs files was not something we ever intended to support—the fact that it ever worked was a bug / unintentional side effect of other work.

@andrewbranch andrewbranch added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 18, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 21, 2024
@SBoudrias
Copy link
Author

Thanks for the answer! I'll review how I dual ship then.

It might be good to document cases where tsc ignore settings. I combed through the documentation first and this behavior didn't appear obvious to me.

@Andarist
Copy link
Contributor

If you were relying on tsc for dual emit then it might be the easiest to switch to using tshy. If you prefer to handroll the solution then I'd recommend switching to .ts files. I've seen you have post-processing step to fixup extensions and stuff - that step could be reused. It feels that if you just tweak its logic a little bit to handle a different set of files and do a slightly different fixup then it should work. Note that I didn't take too deep into your current setup so what I'm saying is not a guarantee but rather a conclusion drawn solely on a first impression of a very tiny slice of your repo ;p

@KpjComp
Copy link

KpjComp commented Oct 13, 2024

Came across this issue using ts.transpileModule(source..

My issue was the resolved file was the .mjs, but my compile chain was also checking for cjs variant, it would load this one if existed, but the filename in transpileModule kept the same extension, also transforming the filename extension fixed this issue.. :)

Of course the above makes total sense, but wonder if your going to ignore module: ts.ModuleKind.CommonJS, it might make sense to throw an exception, it took me a long time to track this bug down. So might help others who have there own compiler chain using Typescript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants