Skip to content

Commit

Permalink
Account for JS file type-only exports
Browse files Browse the repository at this point in the history
  • Loading branch information
JoshuaKGoldberg committed Apr 18, 2023
1 parent e344ec8 commit 5644a4f
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2405,7 +2405,7 @@ export function getCompletionEntriesFromSymbols(
}

// When in a value location in a JS file, ignore symbols that definitely seem to be type-only
if (!isTypeOnlyLocation && isInJSFile(sourceFile) && !(symbol.flags & SymbolFlags.Value) && !isInJSFile(symbol.declarations?.[0]?.getSourceFile())) {
if (!isTypeOnlyLocation && isInJSFile(sourceFile) && symbolAppearsToBeTypeOnly(symbol)) {
continue;
}

Expand Down Expand Up @@ -2503,6 +2503,10 @@ export function getCompletionEntriesFromSymbols(
// expressions are value space (which includes the value namespaces)
return !!(allFlags & SymbolFlags.Value);
}

function symbolAppearsToBeTypeOnly(symbol: Symbol): boolean {

This comment has been minimized.

Copy link
@Andarist

Andarist Apr 18, 2023

Contributor

nit: is the "appearsToBe" bit intentional or are you just unsure if this already covers everything? :p If you are just unsure then I would still use a more "confident" function name - if this function can return some false positives intentionally then the name is fine.

This comment has been minimized.

Copy link
@JoshuaKGoldberg

JoshuaKGoldberg Apr 18, 2023

Author Contributor

Ha, yes, it's intentional. Since we're delving into mixed JS/TS typedefs it felt reasonable to imply this is a little more hand-wavy than completely TS files. But I'm open to a new name if there's one more in line with the TS conventions? 😄

return !(symbol.flags & SymbolFlags.Value) && (!isInJSFile(symbol.declarations?.[0]?.getSourceFile()) || !!(symbol.flags & SymbolFlags.Type));

This comment has been minimized.

Copy link
@Andarist

Andarist Apr 18, 2023

Contributor

I wonder if symbol.declarations part shouldn't refer to .valueDeclaration (perhaps there is a reason why it's not) 🤔

This comment has been minimized.

Copy link
@JoshuaKGoldberg

JoshuaKGoldberg Apr 18, 2023

Author Contributor

symbol.valueDeclaration is undefined in some of the fourslash tests around .js file imports. Example: javascriptModule25.ts.

}
}

function getLabelCompletionAtPosition(node: BreakOrContinueStatement): CompletionInfo | undefined {
Expand Down
14 changes: 14 additions & 0 deletions tests/cases/fourslash/javascriptModulesTypeImportAsValue.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts'/>
// @allowJs: true

// @Filename: types.js
//// /**
//// * @typedef {Object} Pet
//// * @prop {string} name
//// */
//// module.exports = { a: 1 };

// @Filename: app.js
//// import { /**/ } from "./types"

verify.completions({ marker: "", excludes: "Pet" });

0 comments on commit 5644a4f

Please sign in to comment.