Skip to content

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

Merged
sheetalkamat merged 2 commits into
masterfrom
jsonCompletions
May 7, 2018
Merged

Suggest json files in completion when resolveJsonModule is set and module resolution is node#23915
sheetalkamat merged 2 commits into
masterfrom
jsonCompletions

Conversation

@sheetalkamat
Copy link
Copy Markdown
Member

Fixes #23899

@sheetalkamat sheetalkamat requested review from a user and mhegazy May 5, 2018 00:11
Comment thread src/services/pathCompletions.ts Outdated

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

Choose a reason for hiding this comment

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

*supported

Copy link
Copy Markdown

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
Copy Markdown
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 }); No newline at end of file
Copy link
Copy Markdown

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
Copy Markdown
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 }); No newline at end of file
Copy link
Copy Markdown

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