Fxied declaration emit for JS files when module.exports is assigned a non-alias value and when it has extra type members
#56243
Conversation
… a non-alias value and when it has extra type members
There was a problem hiding this comment.
This seems like a good solution. I'd like @weswigham to take a look too.
src/compiler/checker.ts
Outdated
| let exports = arrayFrom(getExportsOfSymbol(symbol).values()); | ||
| const merged = getMergedSymbol(symbol); | ||
| if (merged !== symbol) { | ||
| exports = concatenate(exports, filter(arrayFrom(getExportsOfSymbol(merged).values()), m => !(getSymbolFlags(resolveSymbol(m)) & SymbolFlags.Value))); |
There was a problem hiding this comment.
what happens if you don't concatenate+filter? I thought the exports of merged would suffice to replace symbol's exports entirely. If not, what is missing in merged? Would it make more sense to start from merged's exports and add only specific things from symbol?
There was a problem hiding this comment.
what happens if you don't concatenate+filter?
Please see the diff below:
git diff
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 6ab6160af6..d9084479f3 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -9268,7 +9268,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
let exports = arrayFrom(getExportsOfSymbol(symbol).values());
const merged = getMergedSymbol(symbol);
if (merged !== symbol) {
- exports = concatenate(exports, filter(arrayFrom(getExportsOfSymbol(merged).values()), m => !(getSymbolFlags(resolveSymbol(m)) & SymbolFlags.Value)));
+ exports = arrayFrom(getExportsOfSymbol(merged).values());
}
return filter(exports, m => isNamespaceMember(m) && isIdentifierText(m.escapedName as string, ScriptTarget.ESNext));
}
diff --git a/tests/baselines/reference/jsDeclarationsExportAssignedClassInstance3.js b/tests/baselines/reference/jsDeclarationsExportAssignedClassInstance3.js
index 5f23727d23..fb9234b587 100644
--- a/tests/baselines/reference/jsDeclarationsExportAssignedClassInstance3.js
+++ b/tests/baselines/reference/jsDeclarationsExportAssignedClassInstance3.js
@@ -23,5 +23,37 @@ module.exports.additional = 20;
//// [index.d.ts]
-export let member: number;
-export let additional: 20;
+declare namespace _exports {
+ export { additional };
+}
+declare namespace _exports {
+ let member: number;
+ let additional: 20;
+}
+export = _exports;
+
+
+//// [DtsFileErrors]
+
+
+out/index.d.ts(2,14): error TS2323: Cannot redeclare exported variable 'additional'.
+out/index.d.ts(2,14): error TS2484: Export declaration conflicts with exported declaration of 'additional'.
+out/index.d.ts(6,9): error TS2323: Cannot redeclare exported variable 'additional'.
+
+
+==== out/index.d.ts (3 errors) ====
+ declare namespace _exports {
+ export { additional };
+ ~~~~~~~~~~
+!!! error TS2323: Cannot redeclare exported variable 'additional'.
+ ~~~~~~~~~~
+!!! error TS2484: Export declaration conflicts with exported declaration of 'additional'.
+ }
+ declare namespace _exports {
+ let member: number;
+ let additional: 20;
+ ~~~~~~~~~~
+!!! error TS2323: Cannot redeclare exported variable 'additional'.
+ }
+ export = _exports;
+
\ No newline at end of file
diff --git a/tests/baselines/reference/jsDeclarationsExportAssignmentExpressionPlusSecondary.js b/tests/baselines/reference/jsDeclarationsExportAssignmentExpressionPlusSecondary.js
index 83bf1fb653..32b3b52524 100644
--- a/tests/baselines/reference/jsDeclarationsExportAssignmentExpressionPlusSecondary.js
+++ b/tests/baselines/reference/jsDeclarationsExportAssignmentExpressionPlusSecondary.js
@@ -31,12 +31,50 @@ module.exports.Strings = Strings;
//// [index.d.ts]
-export namespace Strings {
+declare namespace _exports {
+ export { Strings };
+}
+declare namespace _exports {
+ export let thing: string;
+ export let also: string;
+ export namespace desc {
+ let item: string;
+ }
+ export { Strings };
+}
+export = _exports;
+declare namespace Strings {
let a: string;
let b: string;
}
-export declare let thing: string;
-export declare let also: string;
-export declare namespace desc {
- let item: string;
-}
+
+
+//// [DtsFileErrors]
+
+
+out/index.d.ts(2,14): error TS2300: Duplicate identifier 'Strings'.
+out/index.d.ts(10,14): error TS2300: Duplicate identifier 'Strings'.
+
+
+==== out/index.d.ts (2 errors) ====
+ declare namespace _exports {
+ export { Strings };
+ ~~~~~~~
+!!! error TS2300: Duplicate identifier 'Strings'.
+ }
+ declare namespace _exports {
+ export let thing: string;
+ export let also: string;
+ export namespace desc {
+ let item: string;
+ }
+ export { Strings };
+ ~~~~~~~
+!!! error TS2300: Duplicate identifier 'Strings'.
+ }
+ export = _exports;
+ declare namespace Strings {
+ let a: string;
+ let b: string;
+ }
+
\ No newline at end of file
diff --git a/tests/baselines/reference/jsDeclarationsExportDoubleAssignmentInClosure.js b/tests/baselines/reference/jsDeclarationsExportDoubleAssignmentInClosure.js
index 610ded5f10..9bd4e00af5 100644
--- a/tests/baselines/reference/jsDeclarationsExportDoubleAssignmentInClosure.js
+++ b/tests/baselines/reference/jsDeclarationsExportDoubleAssignmentInClosure.js
@@ -27,9 +27,36 @@ function foo() {
//// [index.d.ts]
+declare namespace _exports {
+ export { m as methods };
+}
declare function _exports(o: any): any;
declare namespace _exports {
export { m as methods };
}
export = _exports;
declare function m(): void;
+
+
+//// [DtsFileErrors]
+
+
+out/index.d.ts(2,19): error TS2300: Duplicate identifier 'methods'.
+out/index.d.ts(6,19): error TS2300: Duplicate identifier 'methods'.
+
+
+==== out/index.d.ts (2 errors) ====
+ declare namespace _exports {
+ export { m as methods };
+ ~~~~~~~
+!!! error TS2300: Duplicate identifier 'methods'.
+ }
+ declare function _exports(o: any): any;
+ declare namespace _exports {
+ export { m as methods };
+ ~~~~~~~
+!!! error TS2300: Duplicate identifier 'methods'.
+ }
+ export = _exports;
+ declare function m(): void;
+
\ No newline at end of file
Would it make more sense to start from merged's exports and add only specific things from symbol?
Maybe. Some time has past since I was investigating this and it is a little bit blurry to me at this point. Based on the diff above though we can't do that - we could only add more this way and with the diff above we already return too much.
Perhaps a different fix can be figured out though.
…orts-non-alias-with-type-members
| let exports = arrayFrom(getExportsOfSymbol(symbol).values()); | ||
| const merged = getMergedSymbol(symbol); | ||
| if (merged !== symbol) { | ||
| exports = concatenate(exports, filter(arrayFrom(getExportsOfSymbol(merged).values()), m => !(getSymbolFlags(resolveSymbol(m)) & SymbolFlags.Value))); |
There was a problem hiding this comment.
I think this could potentially have the same symbol in both exports and the merged exports through some complicated aliasing. In such a case, it'd behoove us to deduplicate them. Maybe we could just add 'em both to a Set and then arrayifying it? The deduplicate array helper is... icky - definitely not for things as potentially large as a symbol export table.
There was a problem hiding this comment.
i pushed out the requested change
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the public perf test suite on this PR at 4bba29b. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 4bba29b. You can monitor the build here. |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 4bba29b. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 4bba29b. You can monitor the build here. Update: The results are in! |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Andarist
deleted the
fix/js-dts-module-exports-non-alias-with-type-members
branch
fixes #56229