Skip to content

Fix CJS local binding emit for ES decorators #53725

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

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Fix CJS local binding emit for ES decorators #53725

merged 2 commits into from
Apr 10, 2023

Conversation

rbuckton
Copy link
Contributor

Fixes the emit for the local binding of a class name.

Fixes #53332

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 10, 2023
@@ -208,7 +208,7 @@ let C = (() => {
Object.defineProperty(exports, "__esModule", { value: true });
exports.D = void 0;
/*34*/
exports.D = (() => {
let D = exports.D = (() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this uses a let, will code like:

class Foo {}
namespace Foo {
    export test = 1234;
}

Work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should. The merging behavior occurs in the ts transform, before decorators are processed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test we could add to verify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That behavior is already covered by tests/cases/conformance/internalModules/DeclarationMerging/ClassAndModuleThatMerge*.ts. Nothing I changed here would have any impact on declaration merging since that happens long before this transform runs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I get that class/namespace merging is covered, but skimming the tests you link, none of them export and use CJS, so it doesn't seem like this particular bug + namespace merging is covered.

Am I missing a test that would exhibit both behaviors? I would assume it doesn't exist, otherwise the original bug would have also been caught?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think class/namespace merging has anything to do with the original bug, but I've added a test anyways.

@jakebailey
Copy link
Member

Is this backportable or since isn't a regression from 4.9 to 5.0, we'll just wait for 5.1?

@rbuckton
Copy link
Contributor Author

It can be backported, but whether it should be may be a question for @DanielRosenwasser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decorated classes in TS 5.x can't be used in the same file
3 participants