-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
@@ -208,7 +208,7 @@ let C = (() => { | |||
Object.defineProperty(exports, "__esModule", { value: true }); | |||
exports.D = void 0; | |||
/*34*/ | |||
exports.D = (() => { | |||
let D = exports.D = (() => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Is this backportable or since isn't a regression from 4.9 to 5.0, we'll just wait for 5.1? |
It can be backported, but whether it should be may be a question for @DanielRosenwasser. |
Fixes the emit for the local binding of a class name.
Fixes #53332