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

Named exports missing from CommonJS module compiled with TS 4.4 and importHelpers: true #45813

Closed
maxholman opened this issue Sep 10, 2021 · 14 comments · Fixed by #47070
Closed
Assignees
Labels
Fixed A PR has been merged for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@maxholman
Copy link

maxholman commented Sep 10, 2021

Bug Report

🔎 Search Terms

SyntaxError: Named export not found. The requested module is a CommonJS module, which may not support all module.exports as named exports, esm, exports, __exportStar, tslib

🕗 Version & Regression Information

  • This changed between versions 4.4 and 4.3

⏯ Playground Link

Playground links demonstrate the difference in compiled output between versions

Playground link with relevant code (4.4)

Playground link with relevant code (4.3)

Full reproduction repo and instructions

💻 Code

From the above Playground link, 4.4 produces:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const tslib_1 = require("tslib");
(0, tslib_1.__exportStar)(require("fs"), exports);

4.3 produces:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const tslib_1 = require("tslib");
tslib_1.__exportStar(require("fs"), exports);

Note the use of the comma operator to unbind __exportStar. This seems to be new in 4.4 via more indirect calls for imported functions and #44624

🙁 Actual behavior

Named exports are missing when importing a CommonJS module compiled with Typescript 4.4 when importHelpers is true.

Setting importHelpers to false fixes this problem, but may result in a lot of duplicated code via tslib

🙂 Expected behavior

Named exports should either exist, whether I have importHelpers turned on or off.... or would be good this this was specifically documented as a breaking change.

(possibly) related: #45189

@StanislavKharchenko
Copy link

I second concerned about this.
What is the reason of such breaking changes in 4.4 version?

@BenSjoberg
Copy link

This just caused NestJS to break on a semver-minor change because they updated TS. This makes me worried - a lot of CJS modules are compiled with TypeScript, and this could cause a lot of them to break over the next few months as they upgrade TS. Any chance this change can be reverted for module re-exports?

@andrewbranch
Copy link
Member

Ohh, I totally didn’t get what this issue was about without @BenSjoberg’s context of nodejs/cjs-module-lexer#63. This isn’t a TypeScript issue; the modules produced in 4.3 vs 4.4 are 100% equivalent in runtime behavior since the __exportStar helper doesn’t use a this. The problem is just that Node’s CJS/ESM interop layer no longer understands what we’re doing. They admit that getting named exports out of CJS is a best-guess effort right in the error message:

The requested module is a CommonJS module, which may not support all module.exports as named exports

This is 100% a cjs-module-lexer problem. Thanks @BenSjoberg. Please weigh in at nodejs/cjs-module-lexer#63 if this is causing problems for you.

@andrewbranch andrewbranch added External Relates to another program, environment, or user action which we cannot control. and removed Needs Investigation This issue needs a team member to investigate its status. labels Oct 14, 2021
@andrewbranch andrewbranch removed this from the TypeScript 4.5.1 milestone Oct 14, 2021
@guybedford
Copy link
Contributor

@andrewbranch thanks for the analysis here. We can add a fix, but the project there was always supposed to be stabilized and frozen due to the risk of Node.js versions giving different exports being a highly undebuggable experience for users. I'm still tending towards not pushing a fix for this reason but happy to discuss further in that issue.

@andrewbranch
Copy link
Member

@guybedford that’s totally reasonable, and I wouldn’t mean to imply that I think cjs-module-lexer has a particular obligation to recognize our output even if it wasn’t frozen. It seems like the expectation is that sometimes you’ll just have to use the default import and access properties off of it. My earlier comment was only meant to convey that on the TS side, we also can’t change, as our old emit was just plain wrong and the new emit fixes bugs. Thanks!

unicornware added a commit to flex-development/log that referenced this issue Oct 30, 2021
unicornware added a commit to flex-development/log that referenced this issue Oct 30, 2021
unicornware added a commit to flex-development/log that referenced this issue Oct 30, 2021
unicornware added a commit to flex-development/log that referenced this issue Oct 30, 2021
unicornware added a commit to flex-development/trext that referenced this issue Oct 30, 2021
unicornware added a commit to flex-development/log that referenced this issue Oct 30, 2021
unicornware added a commit to flex-development/log that referenced this issue Oct 30, 2021
@paulmillr
Copy link

Hey folks, I don't understand. How is changing the imports from tslib_1.__exportStar()to (0, tslib_1.__exportStar)() useful? It definitely hurts readability. Isn't it a bug to add 0, ?

@andrewbranch
Copy link
Member

No, it is a bug not to add the indirect call. See #35420.

@StanislavKharchenko
Copy link

@andrewbranch
Do you have some documentation and changelog regarding this?
There was not clear why this was added.

@BenSjoberg
Copy link

BenSjoberg commented Dec 2, 2021

No, it is a bug not to add the indirect call. See #35420.

Would it be possible to add an exception so that this doesn't occur with functions from tslib? None of the functions in tslib use this, so there should be no behavioral differences.

I know how hacky it sounds, but the status quo is causing CommonJS packages written in TypeScript to be somewhat fragile when used from ESM. Unless you eschew named imports entirely when using a CommonJS library (which is a bit frustrating since the TS language service will auto-import them as named imports), you run the risk of your code breaking on a minor or patch update if the developer turns on turns on importHelpers or just rewrites export { Foo, Bar } from './baz' to export * from './baz'. IMO, the fact that doing either of those could lead to breaking changes for some users is very unexpected behavior.

@andrewbranch
Copy link
Member

We’ll investigate that 👍

@andrewbranch andrewbranch reopened this Dec 3, 2021
@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Dec 3, 2021
@typescript-bot
Copy link
Collaborator

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

@andrewbranch andrewbranch reopened this Dec 7, 2021
@andrewbranch andrewbranch removed the External Relates to another program, environment, or user action which we cannot control. label Dec 7, 2021
@BenSjoberg
Copy link

Thank you so much! :)

@trivikr
Copy link

trivikr commented Jan 21, 2022

Is this issue (and fix) going to be added to the TypeScript milestones for 4.6.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed A PR has been merged for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants