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

Cross-file Typescript support in vscode-web #169311

Merged
merged 63 commits into from
Jan 12, 2023
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Dec 15, 2022

This PR adds cross-file Typescript support to vscode-web. Like the desktop version, it starts two tsservers, one in syntax mode and one in semantic mode. This means that you can now import from from other files without having them open in the editor.

It does this by changing the web-based tsserver host, which now depends on vscode-wasm to communicate with a virtual file system. The code is substantially similar to before, but supports slightly more of the tsserver API, and adds a translation layer for talking to a virtual file system based on URIs instead of a real one based on paths. It also has adapter code for responding to watch events asynchronously but communicating the event to tsserver sychronously.

It makes some important changes to the typescript extension:

  1. WorkerServerProcess in serverProcess.browser.ts now imports vscode-wasm packages and sets up several MessageChannels with the tsserver host.
  2. Cancellation support is added in ProcessBasedTsServer in src/tsServer/server.ts.
  3. WorkerServerProcess passes the extensionURI to the tsserver host so that lib*d.ts files can be redirected to their CDN location (the code is copied from typescriptServiceClient.toResource).

I don't know if any of these are done in the right place, or in the right way. I also have some questions about the host implementation:

For @sheetalkamat:

  • require -- is this needed? In TS, it's only used in project system. Answer: No, only needed for node environments.
  • trace -- is this needed? In TS, it's only used in project system. Answer: No.
  • useCaseSensitiveFileNames -- old version says 'false' is the safest option, but the virtual fs is case sensitive. Is false still better? Answer: match the virtual fs: case-sensitive.

For @jrieken:

  • the node code manually skips '.' and '..' in the array returned by fs.readdirSync. Will readDirectory ever return '.' and '..'?
  • writeOutputIsTTY -- I'm using apiClient.vscode.terminal.write -- is it a tty?
  • getWidthOfTerminal -- I don't know where to find this on apiClient.vscode.terminal either
  • clearScreen -- the node version of the code writes \x1BC to the terminal. Would this work for vscode?
  • readFile/writeFile -- the node version handles utf8, utf16le and manually converts big-endian to utf16 little-endian. How does the in-memory filesystem handle this? There's no place to specify encoding.
  • realpath/getDirectories/readDirectory -- how do I resolve symlinks? I copied some fs.statSync-based code for getDirectories, but I don't think it's right.

Not sure who will know these:

  • resolvePath -- node version uses path.resolve. Is it OK to use that? Or should I re-implement it? Just use identity like the old web code?
  • createSHA256Hash -- the browser version is async, so I skipped it. Is this OK?

This PR still needs some cleanup, which I will do next:

  • cancellation should only retain one cancellation checker
    • the one that matches the current request id
    • but that means tracking (or retrieving from tsserver) the request id (aka seq?)
    • and correctly setting/resetting it on the cancellation token too.
  • Cancellation code in vscode is suspiciously prototypey.
    • Specifically, it adds the vscode-wasm cancellation to original cancellation code, but should actually switch to the former for web only.
    • looks like isWeb() is a way to check for being on the web
  • create multiple watchers
    • on-demand instead of watching everything and checking on watch firing
  • Need to parse and pass args through so that the syntax server isn't hard-coded to actually be another semantic server
  • clear out TODOs
  • think about implementing all the other ServerHost methods
    • also copy details from previous implementation (although it's syntax-only so only covers part)
  • organise webServer.ts into multiple files (in a separate PR, to minimise churn)
  • add semicolons everywhere; vscode's lint doesn't seem to complain, but the code clearly uses them

These are tracked in the PR itself in extensions/typescript-language-fetaures/web/README.md

Because I dont have the default browser set up correctnly on any of my
machiens
And some logging as I figure out how to use it
Use webpack's CopyPlugin transform pattern to do this manually. This is
probably a bad idea! It's just for prototyping purposes.
Nonetheless required for it to work!
Another typo. Guess my local test wasn't running the contents really
Also:
- tsserver.js is no longer minified
- log crossOriginIsolated
Build only the ts parts of tsserver.web.js, don't rebuild the vscode
extension. This is a lot faster.
Sheetal showed me the correct way to create a verbose logger instead.
And make isWeb support semantic mode.
Also paste in some example code for cancellation, which is not finished
at all.
1. Remove a little logging.
2. Correct failure return value for getFileSize
3. Reorder some methods and parameters.
I'm not sure if it's OK to depend on a module from another extension;
it's probably better to include the files from a central place instead.
@sandersn sandersn dismissed a stale review via 0288f0f January 6, 2023 22:24
1. Copy and adapt implementations from node host where possible.
2. Note questions for the PR elsewhere.
3. Remove logging except for caught exceptions.
@sandersn sandersn dismissed a stale review via 1c52093 January 9, 2023 19:40
mjbvz and others added 10 commits January 9, 2023 17:25
Also gate it behind a check to `crossOriginIsolated`
It won't minimise it *much*, but I also consolidated some
unnecessarily-spread-out code that will be easier to read in the long
term, and possibly easier to read in diff form as well.
Copy from markdown extension to typescript extension. I used the
existing dependencies in the typescript extension, but verified that
they would work the same.
…y prefix when project wide intellisense is disabled
Symlinks are implicitly supported by the filesystem right now.
@sandersn
Copy link
Member Author

sandersn commented Jan 12, 2023

There are 3 build failures right now:

  1. Windows tests fail installing playwright.
  2. yarn.lock changes aren't allowed.
  3. references to navigator and crossOriginIsolated require either lib="dom" or lib="webworker". Some builds, like those from yarn compile and yarn compile-web have this because they include typescript-language-extension/web/webServer.ts, but something in the Code OSS CI check does not.

@mjbvz I think only the last failure needs to be fixed. I'm investigating but you might be faster than me.

Edit: The failing task is extensions-ci

There might be an established way to add dependencies that's not PRs, in which case (2) will need to be addressed that way.

@mjbvz mjbvz enabled auto-merge (squash) January 12, 2023 19:27
@mjbvz mjbvz merged commit 3261c7d into microsoft:main Jan 12, 2023
@sandersn sandersn deleted the wasm-typescript branch January 12, 2023 20:32
@jrieken
Copy link
Member

jrieken commented Jan 13, 2023

🥳

@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2023
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.

4 participants