Skip to content

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
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.

@rbuckton rbuckton merged commit 3e76fb5 into main Apr 10, 2023
@rbuckton rbuckton deleted the fix53332 branch April 10, 2023 21:05
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

4 participants