Skip to content
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.

Sign up for GitHub

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

Fxied declaration emit for JS files when module.exports is assigned a non-alias value and when it has extra type members #56243

Merged
merged 3 commits into from
Jan 11, 2024
Merged

Fxied declaration emit for JS files when module.exports is assigned a non-alias value and when it has extra type members #56243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
806de6d
Fxied declaration emit for JS files when `module.exports` is assigned…
Andarist Oct 28, 2023
d095ccd
Merge remote-tracking branch 'origin/main' into fix/js-dts-module-exp…
Andarist Dec 26, 2023
4bba29b
use `Set` before converting to an array to avoid potential duplicates
Andarist Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8664,7 +8664,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
});
let addingDeclare = !bundled;
const exportEquals = symbolTable.get(InternalSymbolName.ExportEquals);
if (exportEquals && symbolTable.size > 1 && exportEquals.flags & SymbolFlags.Alias) {
if (exportEquals && symbolTable.size > 1 && exportEquals.flags & (SymbolFlags.Alias | SymbolFlags.Module)) {
symbolTable = createSymbolTable();
// Remove extraneous elements from root symbol table (they'll be mixed back in when the target of the `export=` is looked up)
symbolTable.set(InternalSymbolName.ExportEquals, exportEquals);
Expand Down Expand Up @@ -9197,8 +9197,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function getNamespaceMembersForSerialization(symbol: Symbol) {
const exports = getExportsOfSymbol(symbol);
return !exports ? [] : filter(arrayFrom(exports.values()), m => isNamespaceMember(m) && isIdentifierText(m.escapedName as string, ScriptTarget.ESNext));
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)));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
return filter(exports, m => isNamespaceMember(m) && isIdentifierText(m.escapedName as string, ScriptTarget.ESNext));
}

function isTypeOnlyNamespace(symbol: Symbol) {
Expand Down
35 changes: 35 additions & 0 deletions tests/baselines/reference/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//// [tests/cases/compiler/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.ts] ////

//// [index.js]
/**
* @typedef Options
* @property {string} opt
*/

/**
* @param {Options} options
*/
module.exports = function loader(options) {}


//// [index.js]
"use strict";
/**
* @typedef Options
* @property {string} opt
*/
/**
* @param {Options} options
*/
module.exports = function loader(options) { };


//// [index.d.ts]
declare namespace _exports {
export { Options };
}
declare function _exports(options: Options): void;
export = _exports;
type Options = {
opt: string;
};
18 changes: 18 additions & 0 deletions ...selines/reference/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//// [tests/cases/compiler/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.ts] ////

=== index.js ===
/**
* @typedef Options
* @property {string} opt
*/

/**
* @param {Options} options
*/
module.exports = function loader(options) {}
>module.exports : Symbol(module.exports, Decl(index.js, 0, 0))
>module : Symbol(export=, Decl(index.js, 0, 0))
>exports : Symbol(export=, Decl(index.js, 0, 0))
>loader : Symbol(loader, Decl(index.js, 8, 16))
>options : Symbol(options, Decl(index.js, 8, 33))

20 changes: 20 additions & 0 deletions ...baselines/reference/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//// [tests/cases/compiler/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.ts] ////

=== index.js ===
/**
* @typedef Options
* @property {string} opt
*/

/**
* @param {Options} options
*/
module.exports = function loader(options) {}
>module.exports = function loader(options) {} : (options: Options) => void
>module.exports : (options: Options) => void
>module : { exports: (options: Options) => void; }
>exports : (options: Options) => void
>function loader(options) {} : (options: Options) => void
>loader : (options: Options) => void
>options : Options

16 changes: 16 additions & 0 deletions tests/cases/compiler/jsDeclarationEmitExportAssignedFunctionWithExtraTypedefsMembers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// @strict: true
// @checkJs: true
// @declaration: true
// @outDir: out

// @filename: index.js

/**
* @typedef Options
* @property {string} opt
*/

/**
* @param {Options} options
*/
module.exports = function loader(options) {}
Loading