TS 4.4.0-beta: #44624 change breaks existing code in CommonJS emit

[DavidANeil]

Bug Report

🔎 Search Terms

TS 4.4 beta commonjs

🕗 Version & Regression Information

• This changed between versions 4.3.5 and 4.4.0-beta

⏯ Playground Link

Playground link with relevant code

💻 Code

import {render} from 'less';

render('input', () => {});

🙁 Actual behavior

render is called with a this of Window
Emit is

(0, less_1.render)('input', () => { });

🙂 Expected behavior

render is called with a this of require('less')
Emit should be

less_1.render('input', () => { });

It looks like this was caused by #44624.

[DavidANeil]

To be totally honest, the new behavior feels correct, but the reality is that this just breaks an unknown amount of existing code.

[andrewbranch]

The new behavior is correct, and I thought we did some kind of analysis to see how breaky it would be, but I can’t find that. It at least should be mentioned in the breaking changes section of the release notes (it’s missing from the beta notes). CC @DanielRosenwasser @rbuckton.

[JaredNeil]

Is it also Working as Intended that these two snippets have different behavior?

import * as less from 'less';
less.render('input', () => {});

import {render} from 'less';
render('input', () => {});

[andrewbranch]

I mean, it’s expected. The weird thing is that under the actual implemented CJS/ESM interop in Node v12+, neither of these will work. The only correct way to write an ESM import for this particular CJS module is import less from "less". So in some ways it’s pretty unfortunate that we let you write these at all, but we’ve been doing that for longer than Node’s ESM implementation has existed. The fact is, it’s an accident that import { render } from "less" ever worked at runtime, but now that it doesn’t, it does become a real problem that we can’t tell you that it’s not going to work at check time. 🤔

[fatcerberus]

For the record, in an actual native ESM implementation where less is an ESM module and has the correct exports (specifically render), those two snippets would indeed have different behavior at runtime.

[andrewbranch]

Yeah, totally. The behavior seen in 4.4 looks completely expected, if you had some reason to believe that the receiver of render is supposed to be less. The problem is, that’s not obvious at all, and in fact we have no way of looking at the typings and determining that you can’t/shouldn’t use a named import here.

[DavidANeil]

Yeah I think the emit change is acceptable, but this should definitely be highlighted as a breaking change that affects the runtime. Most breaking changes just mean broken type checking, but given that this can break runtimes, and the type system won't catch it, I think it deserves bold lettering.

[rbuckton]

The problem with less is that its type definitions make it seem like you can use import * as less from 'less' or import { render} from 'less', but the way the actual code for less is written you should be using import less = require("less") or import less from "less" with --esModuleInterop.

Our emit now matches the emit of BabelJS, where you would also get an issue if you tried using import { render } from 'less'.

[andrewbranch]

That’s true to an extent, but AFAIK there’s no way to change less’s type definitions to make import { render } from "less" be an error under --module=commonjs (and still accurately describe the module). Even if it was 100% clear that the export is a class instance and render is an instance method, that kind of import is legal.

// @filename: node_modules/less/index.d.ts
declare class Less {
  render(input: string): void;
}
declare const less: Less;
export = less;

// @filename: test.ts
import { render } from "less"; // no error
render("");

[rbuckton]

We could change the definition of render to have a this argument. Which would at least make the problem somewhat clearer on the usage side.