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.

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

Resolve JS imports from html script tags #193590

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dpikt
Copy link

@dpikt dpikt commented Sep 20, 2023

Partially addresses #26338 by allowing external scripts to be resolved via TSServer. This is based on the great work by @orta in #121517 - I've rearranged things a little bit for a smaller diff.

This enables display of quick info on hover, as well as jump-to-definition for JS imports. This also seems to support //@ts-check, although I haven't tested this thoroughly.

Example JS module:

Screen Shot 2023-09-20 at 10 32 06 AM

Before change (no quick info):

Screen Shot 2023-09-20 at 10 32 01 AM

After change (shows quick info):

Screen Shot 2023-09-20 at 10 22 00 AM

Type checking example:

Screen Shot 2023-09-20 at 10 37 42 AM

Current known limitations:

  • Does not support absolute paths / custom baseUrls
  • Does not show full JSDoc info in quick info

If the overall approach looks good I can add tests for this enhancement. Any and all feedback welcome, thanks!

@dpikt
Copy link
Author

dpikt commented Sep 20, 2023

@microsoft-github-policy-service agree

}
return '1'; // default lib an jquery.d.ts are static
},
getScriptSnapshot: (fileName: string) => {
let text = '';
if (fileName === currentTextDocument.uri) {
text = currentTextDocument.getText();
} else if (isFileURI(fileName) && ts.sys.fileExists(uriToFilePath(fileName))) {
Copy link
Author

Choose a reason for hiding this comment

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

If the requested file is on a local file system, read it with ts.sys.readFile. (Web IDE support is not possible since TS requires sync access, as discussed in #121517)

return definition.filter(d => d.fileName === jsDocument.uri).map(d => {
return definition.map(d => {
const text = jsLanguageService.getProgram()!.getSourceFile(d.fileName)!.text;
const doc = TextDocument.create('tmp', 'javascript', 1, text);
Copy link
Author

@dpikt dpikt Sep 20, 2023

Choose a reason for hiding this comment

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

Convert source files into TextDocument objects to do text range conversion. (There may be a better way to do this).

@dpikt
Copy link
Author

dpikt commented Sep 23, 2023

Update - added tests. Again based on #121517

@@ -64,7 +64,7 @@ export async function testCompletionFor(value: string, expected: { count?: numbe

const list = await mode.doComplete!(document, position, context);

if (expected.count) {
if (expected.count !== undefined) {
Copy link
Author

Choose a reason for hiding this comment

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

Prevent silent errors when testing for count: 0

@orta
Copy link
Contributor

orta commented Nov 25, 2023

It'd be cool to get this in

@friuns2
Copy link

friuns2 commented Mar 25, 2024

any way to add non module js files support? like shown in #26338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants