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

VS Code / TypeScript integration isn't configured correctly for notebook cells #13

Closed
jaked opened this issue Apr 25, 2024 · 22 comments
Closed

Comments

@jaked
Copy link
Collaborator

jaked commented Apr 25, 2024

  • doesn't use project tsconfig.json
  • has JSX configured wrong (so gives an error about React being a UMD global)
  • can't find imported files (works OK for imported packages)

Advice from @Krzysztof-Cieslak about debugging:

Anyway... I guess if I'd want to debug this I'd start with following steps:
Set typescript.tsserver.enableTracing to true
Set typescript.tsserver.log to verbose
Reload the window
Go to TypeScript channel in the Output tab
There you should have path to the log file
Open file in your notebook and open normal TS file to generate some logs.
Compare the differences in the log file

@jaked
Copy link
Collaborator Author

jaked commented May 17, 2024

here are some relevant-looking log messages when you create a new cell:

Info 2074 [12:32:59.988] Search path: ^/vscode-notebook-cell/ts-nul-authority/Users/jaked/repos/react-ts-example-2
Info 2075 [12:32:59.988] For info: ^/vscode-notebook-cell/ts-nul-authority/Users/jaked/repos/react-ts-example-2/test.vnb#W3sZmlsZQ== :: No config files found.
Info 2076 [12:32:59.988] Starting updateGraphWorker: Project: /dev/null/inferredProject1*
Err 2077  [12:32:59.989] languageId not found for ^/vscode-notebook-cell/ts-nul-authority/Users/jaked/repos/react-ts-example-2/test.vnb#W3sZmlsZQ==
Err 2078  [12:32:59.989] languageId not found for ^/vscode-notebook-cell/ts-nul-authority/Users/jaked/repos/react-ts-example-2/test.vnb#W3sZmlsZQ==
Info 2079 [12:32:59.997] Finishing updateGraphWorker: Project: /dev/null/inferredProject1* projectStateVersion: 54 projectProgramVersion: 9 structureChanged: true structureIsReused:: Not Elapsed: 9.251708000898361ms
Info 2080 [12:32:59.998] Project '/dev/null/inferredProject1*' (Inferred)

Judging from the way cells are assigned filenames, I think maybe imports of local files can't be found because the relative path isn't correct? Yup, if I give an absolute path it works.

Also maybe that's why config files aren't found? There's also these log messages:

Info 2122 [12:37:58.438] request:
    {
      "seq": 326,
      "type": "request",
      "command": "projectInfo",
      "arguments": {
        "file": "^/vscode-notebook-cell/ts-nul-authority/Users/jaked/repos/react-ts-example-2/test.vnb#W2sZmlsZQ==",
        "needFileNameList": false
      }
    }
Perf 2123 [12:37:58.441] 326::projectInfo: elapsed time (in milliseconds) 3.6483
Info 2124 [12:37:58.441] response:
    {"seq":0,"type":"response","command":"projectInfo","request_seq":326,"success":true,"body":{"configFileName":"/dev/null/inferredProject1*","languageServiceDisabled":false}}

which suggests that it's inferring a config file (because it can't find one relative to the path).

I wonder if there was some TypeScript support for these notebook pathnames that got broken somehow.

@jaked
Copy link
Collaborator Author

jaked commented May 17, 2024

VS Code understands files starting with ^ to refer to in-memory resources, see

https://github.com/microsoft/vscode/blob/1.87.2/extensions/typescript-language-features/src/typescriptServiceClient.ts#L763

@jaked
Copy link
Collaborator Author

jaked commented May 20, 2024

semanticDiag is an event sent from TypeScript to indicate a problem, e.g. when the imported file can't be found:

    {"seq":0,"type":"event","event":"semanticDiag","body":{"file":"^/vscode-notebook-cell/ts-nul-authority/Users/jaked/repos/react-ts-example-2/test.vnb#W2sZmlsZQ==","diagnostics":[{"start":{"line":1,"offset":21},"end":{"line":1,"offset":32},"text":"Cannot find module './test.ts' or its corresponding type declarations.","code":2307,"category":"error"}]}}

@jaked
Copy link
Collaborator Author

jaked commented May 20, 2024

here's a relevant-looking log message from the TypeScript server (I think?!)

{"pid":1,"tid":1,"ph":"B","cat":"check","ts":243237142666.625,"name":"checkSourceFile","args":{"path":"/users/jaked/repos/react-ts-example-2/^/vscode-notebook-cell/ts-nul-authority/users/jaked/repos/react-ts-example-2/test.vnb#w3szmlszq=="}},

what's going on with the path there? guessing that's constructed client-side. I think actually it's getting the root path prepended server-side because it starts with ^.

@jaked
Copy link
Collaborator Author

jaked commented May 20, 2024

seems like TypeScript in notebook cells isn't well-supported 😢

see microsoft/vscode#212367 disabling error highlighting in cells.

see DonJayamanne/typescript-notebook#79 (comment) microsoft/vscode#186811 microsoft/vscode#188186 about cell paths not working

@jaked
Copy link
Collaborator Author

jaked commented May 20, 2024

see microsoft/vscode#183688 microsoft/vscode#183685 about finding tsconfig.json

@jaked
Copy link
Collaborator Author

jaked commented May 20, 2024

ok looks like TypeScript understands ^ to mean that the file is "dynamic" (not present in the filesystem I think), see https://github.com/microsoft/TypeScript/blob/main/src/server/scriptInfo.ts#L348

(not sure why it is prepending the project root in that case)

@jaked
Copy link
Collaborator Author

jaked commented May 20, 2024

I think dynamic files are supposed to look in the project root for their tsconfig.json, see microsoft/TypeScript#35111

@jaked
Copy link
Collaborator Author

jaked commented May 21, 2024

the path in checkSourceFile above seems to be set here (prepending the root path since the filename starts with ^) https://github.com/microsoft/TypeScript/blob/main/src/services/services.ts#L1865

@jaked
Copy link
Collaborator Author

jaked commented May 21, 2024

missing file error is defined here:

https://github.com/microsoft/TypeScript/blob/main/src/compiler/diagnosticMessages.json#L1887

(I think this gets compiled into a code file which is referenced from checker.ts)

called here (in checker.ts):

    function resolveExternalModuleName(location: Node, moduleReferenceExpression: Expression, ignoreErrors?: boolean): Symbol | undefined {
        const isClassic = getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.Classic;
        const errorMessage = isClassic ?
            Diagnostics.Cannot_find_module_0_Did_you_mean_to_set_the_moduleResolution_option_to_nodenext_or_to_add_aliases_to_the_paths_option
            : Diagnostics.Cannot_find_module_0_or_its_corresponding_type_declarations;
        return resolveExternalModuleNameWorker(location, moduleReferenceExpression, ignoreErrors ? undefined : errorMessage);
    }

@jaked
Copy link
Collaborator Author

jaked commented May 21, 2024

looks like resolveExternalModuleName checks a previously-initialized map of import names, which is computed here https://github.com/microsoft/TypeScript/blob/27bcd4cb5a98bce46c9cdd749752703ead021a4b/src/compiler/program.ts#L4039

@jaked
Copy link
Collaborator Author

jaked commented May 21, 2024

resolving import names boils down to https://github.com/microsoft/TypeScript/blob/27bcd4cb5a98bce46c9cdd749752703ead021a4b/src/compiler/program.ts#L2050 which doesn't consider dynamic paths afaict

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

I hacked around it by checking for the magic notebook cell pathname, and in that case passing through the underlying notebook pathname:

    function resolveModuleNamesWorker(moduleNames: readonly StringLiteralLike[], containingFile: SourceFile, reusedNames: readonly StringLiteralLike[] | undefined): readonly ResolvedModuleWithFailedLookupLocations[] {
        if (!moduleNames.length) return emptyArray;
        const cellPrefix = "^/vscode-notebook-cell/ts-nul-authority";
        const containingFileName =  
            containingFile.originalFileName.startsWith(cellPrefix) ?
            containingFile.originalFileName.substring(cellPrefix.length) :
            getNormalizedAbsolutePath(containingFile.originalFileName, currentDirectory);
        ...

and now

Image

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

looks like TypeScript infers a project (called inferredProject{N}) and configuration when it can't find a tsconfig.json. the inferred project name is generated here https://github.com/microsoft/TypeScript/blob/27bcd4cb5a98bce46c9cdd749752703ead021a4b/src/server/utilitiesPublic.ts#L124

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

when searching for a config file from a file path, dynamic files are skipped https://github.com/microsoft/TypeScript/blob/27bcd4cb5a98bce46c9cdd749752703ead021a4b/src/server/editorServices.ts#L2234

but it seems like dynamic files are supposed to use the root tsconfig.json (root where tsserver was run?) per microsoft/TypeScript#35111

no the code above that skips dynamic files is part of microsoft/TypeScript#35111; I think the intent is to have an inferred project.

the meaning of "dynamic" files seems overloaded; for notebook cells there is an underlying file so maybe they shouldn't be dynamic. but cells are separate documents, and there needs to be a way for VS Code to find the cell to report errors, which is why the URI is encoded as a dynamic filename.

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

I think in assignProjectToOpenedScriptInfo https://github.com/microsoft/TypeScript/blob/27bcd4cb5a98bce46c9cdd749752703ead021a4b/src/server/editorServices.ts#L3768 we want to check for magic notebook cell pathnames and use the underlying path when we call getConfigFileNameForFile.

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

ok I tried that and now it finds tsconfig.json and doesn't put squigglies on React stuff

Image

@jaked
Copy link
Collaborator Author

jaked commented May 22, 2024

huh the VS Code status bar still thinks there's no tsconfig.json

Image

@jaked
Copy link
Collaborator Author

jaked commented May 23, 2024

db7a140 documents using this hacked TypeScript

going to close this, I'll see about upstreaming the fixes somehow.

@jaked jaked closed this as completed May 23, 2024
@jaked
Copy link
Collaborator Author

jaked commented May 28, 2024

see also microsoft/vscode#196172

@jaked
Copy link
Collaborator Author

jaked commented May 29, 2024

filed microsoft/vscode#213740

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

No branches or pull requests

1 participant