-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Add isolatedModules error for ambiguous imports referenced in decorator metadata #42915
Conversation
If I understand correctly, the issue is that we keep in a static import for the interface: import { X, B } from "./other";
class C extends B {
@dec x: X;
} We try to emit import { X, B } from "./other";
class C extends B {
}
__decorate([
dec,
__metadata("design:type", typeof (_a = typeof X !== "undefined" && X) === "function" ? _a : Object)
], C.prototype, "x", void 0); However, that leaves in a static import for import { B } from "./other";
import * as other_1 from "./other"; // replace `import { X }` with namespace import...
class C extends B {
}
__decorate([
dec,
__metadata("design:type", typeof (_a = typeof other_1.X !== "undefined" && other_1.X) === "function" ? _a : Object)
], C.prototype, "x", void 0); |
This doesn't occur in CJS, AMD, or System emit because we always end up with a "namespace" import in those cases: const other_1 = require("./other");
class C {
}
__decorate([
dec,
__metadata("design:type", typeof (_a = typeof other_1.X !== "undefined" && other_1.X) === "function" ? _a : Object)
], C.prototype, "x", void 0); |
src/compiler/diagnosticMessages.json
Outdated
@@ -879,6 +879,10 @@ | |||
"category": "Error", | |||
"code": 1266 | |||
}, | |||
"A type referenced in a decorated signature must be imported with 'import type' when 'isolatedModules' and 'emitDecoratorMetadata' are enabled.": { |
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.
Should we consider adding an import type
quick-fix for this?
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.
Will add if we decide to keep this approach after discussing in a design meeting
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.
Added, along with a fix to convert named imports a namespace import, if it makes sense.
I’m not sure where the design meeting notes are where we discussed this, but I believe the compromise we came up with was
|
I think to be safe, this shouldn’t go in post-beta, so will plan to target for 4.5. |
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.
Looks good to me. I think now is a good time to put it into 4.7 too.
@@ -7111,6 +7115,7 @@ | |||
"code": 95166 | |||
}, | |||
|
|||
|
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.
this line seems extraneous
Fixes #42281
Teeeechnically maybe a breaking change since there’s a new error, but it should only be an error when a transpiler would produce code that errors at runtime.