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

Include non-enumerable keys in __importStar helper #60262

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Include non-enumerable keys in __importStar helper #60262

merged 1 commit into from
Oct 18, 2024

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Oct 17, 2024

This changes the __importStar helper to include non-enumerable own keys to ensure the following two cases are consistent:

// ts
import A, { b } from "mod";
A;
b;

// js
const mod_1 = __importStar(require("mod"));
mod_1.default;
mod_1.b;

vs

// ts
import A from "mod";
import { b } from "mod";
A;
b;

// js
const mod_1 = __importDefault(require("mod"));
const mod_2 = require("mod");
mod_1.default;
mod_2.b;

The discrepancy occurs when the module "mod" produces exports that are non-enumerable, such as:

class C {
  static b() {}
}

module.exports = C;

In the above case, C.b is an own static method of C which makes it non-enumerable by default.

The other way to address this would be to also wrap mod_2's require in __importStar so that we apply the rule consistently in both cases, but that would be a major breaking change as it has the potential to break existing users w/o warning.

The companion PR for tslib can be found at microsoft/tslib#272.

Fixes #45133

@rbuckton rbuckton merged commit 2e4f2c7 into main Oct 18, 2024
32 checks passed
@rbuckton rbuckton deleted the fix45133 branch October 18, 2024 14:26
@jakebailey
Copy link
Member

Was there a typo? #41533 seems unrelated.

Should this have waited for 5.8 since it's an emit change?

@rbuckton
Copy link
Member Author

It's a bugfix to an emit helper and I would say the risk of it being an issue is fairly small given that it is more permissive. Going the other direction I mentioned in the OP would have needed to wait for 5.8 as it would be a breaking change.

@rbuckton
Copy link
Member Author

Was there a typo? #41533 seems unrelated.

Yeah, that should have been #45133.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

esModuleInterop: true with class static method
4 participants