Skip to content

Suggest json files in completion when resolveJsonModule is set and module resolution is node #23915

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

Merged
merged 2 commits into from
May 7, 2018

Conversation

sheetalkamat
Copy link
Member

Fixes #23899

@sheetalkamat sheetalkamat requested review from a user and mhegazy May 5, 2018 00:11
@@ -27,7 +27,7 @@ namespace ts.Completions.PathCompletions {
const scriptDirectory = getDirectoryPath(scriptPath);

if (isPathRelativeToScript(literalValue) || isRootedDiskPath(literalValue)) {
const extensions = getSupportedExtensions(compilerOptions);
const extensions = getSupportExtensionsForModuleResolution(compilerOptions);
Copy link

Choose a reason for hiding this comment

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

*supported

Copy link

Choose a reason for hiding this comment

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

Shouldn't we just change getSupportedExtensions? That's used by isSupportedSourceFileName which is used by watch.ts, and presumably we want to watch for changes to '.json' files if they're included in the program?

Copy link
Member Author

Choose a reason for hiding this comment

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

isSupportedSourceFileName which internally uses getSupportedExtensions is used to see if the file name is valid when getting the fileNames to compile. We do not add json files then and we shouldnt because we only want to add them through module resolution.

// @Filename: /project/index.ts
////import { } from ".//**/";

verify.completionsAt("", [], { isNewIdentifierLocation: true });
Copy link

Choose a reason for hiding this comment

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

Why doesn't this have a completion?

Copy link
Member Author

Choose a reason for hiding this comment

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

because module resolution strategy isnt "node" and hence json files wont be resolved.

// @Filename: /project/index.ts
////import { } from "/**/";

verify.completionsAt("", ["test.json"], { isNewIdentifierLocation: true });
Copy link

Choose a reason for hiding this comment

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

Nit: It would more realistically be node_modules/package-name/index.json.

@sheetalkamat sheetalkamat merged commit 8fc4242 into master May 7, 2018
@sheetalkamat sheetalkamat deleted the jsonCompletions branch May 7, 2018 19:53
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant