Refresh Images In Markdown Preview On Change

[hediet]

This PR fixes #65258.

The tests are still WIP as I haven't been able to figure out how to run them.
Running ./scripts/test ended with errorlevel: 0. yarn run mocha --run extensions\markdown-language-features\src\test\engine.test.ts failed as it could not find the javascript source. yarn run mocha --run extensions\markdown-language-features\dist\test\engine.test.js failed with "exports is not defined".
After seeing the CI, I noticed that it is an integration test and then found the launch configuration of the markdown test.

Demo:

Key of this implementation is this code:

// NEW: Return both the html and a list of all images.
export interface RenderOutput {
  html: string;
  containingImages: { src: string }[];
}

// NEW: The context provides cache keys for images.
export interface RenderContext {
  /**
   * Each image is identified by its "src".
   * Two images with the same src but different cache keys must not share the
   * same cache entry.
   */
  imageCacheKeyBySrc?: ReadonlyMap</* src: */ string, string>;
}

class MarkdownEngine {
  ...
  // NEW: Accept context to get image cache keys.
  // NEW: Return RenderOutput rather than string to communicate the src of all images.
  public async render(input: SkinnyTextDocument | string, context: RenderContext = {}): Promise<RenderOutput> {
    ...
  }
  ...
}

...


class MarkdownPreview {
  ...
  private async updatePreview(forceUpdate?: boolean): Promise<void> {
    ...
    // NEW: Pass this._imageCacheKeysBySrc here and content is not a string anymore
    const content = await this._contentProvider.provideTextDocumentContent(document, this,
      this._previewConfigurations, this.line, this._imageCacheKeysBySrc, this.state);

    // Another call to `doUpdate` may have happened.
    // Make sure we are still updating for the correct document
    if (this.currentVersion?.equals(pendingVersion)) {
      this.setContent(content);
    }
  }
  ...

  private setContent(content: MarkdownContentProviderOutput): void {
    ...
    // NEW: Synchronize file watchers with (relative) images in markdown

    const srcs = new Set(content.containingImages.map(img => img.src));

    // Delete stale file watchers.
    for (const [src, watcher] of [...this._fileWatchersBySrc]) {
      if (!srcs.has(src)) {
        watcher.dispose();
        this._fileWatchersBySrc.delete(src);
      }
    }

    // Create new file watchers.
    const root = vscode.Uri.joinPath(this._resource, '../');
    for (const src of srcs) {
      const uri = urlToFileUri(src, root);
      if (uri && !this._fileWatchersBySrc.has(src)) {
        const watcher = vscode.workspace.createFileSystemWatcher(uri.fsPath);
        watcher.onDidChange(() => {
          this.currentCacheKeyOffset++;
          this._imageCacheKeysBySrc.set(src, `${this.currentCacheKeyOffset}`);
          this.refresh();
        });
        this._fileWatchersBySrc.set(src, watcher);
      }
    }
  }
  ...
}

[ghost]

All CLA requirements met.

[mjbvz]

Thanks for the PR.

I've looked it over and provided feedback. My main question is if there is a different way to bust the browser image cache for these cases

[mjbvz]

This import will fail in browsers. I think you can instead what I do here:

vscode/extensions/simple-browser/src/extension.ts

Line 10 in 6cceb4e

declare const URL: typeof import('url').URL;

This will let you use the globally defined URL object that ships in both browsers and node

[hediet]

Thanks, I changed that. Are the tests in markdown-language-features run in both node and browser environments?
Edit: I don't think so, otherwise CI would have detected that this import will crash.

I used to think even when VS Code runs in the browser, extensions have to be run inside a node environment.

[mjbvz]

We may be on a virtual file system, so we probably should not restrict this to 'file' uris. Instead you can use isWritableFilesystem to see if a given scheme is known to VS Code

[hediet]

But createFileSystemWatcher seems to work on file paths only, doesn't it?
I generalized the function, but changed the consumer to this:

const uri = urlToUri(src, root);
if (uri && uri.scheme === 'file' && !this._fileWatchersBySrc.has(src)) {
    const watcher = vscode.workspace.createFileSystemWatcher(uri.fsPath);
    // ...
}

[mjbvz]

Looks like you are using the query string make sure we don't used cached image. I wonder if VS Code should be doing this automatically though by looking at etags or similar for the resource?

Have you debugged to see why the images are being cached in the first place? Are vscode-webview-resource request perhaps not setting the correct cache headers?

[hediet]

A very good idea. I changed the approach and got rid of the cache keys. Instead, responses to vscode-webview-resource requests have an etag and a cache-control header now. Also, if-none-match is implemented.

Basically it is a pull approach now - on every possible change, all resources with an etag are re-requested. However, responses are cached.
Before it was a push approach - only if an image actually changed, it got re-requested.

The advantages of this vs cache keys are:

• More natural code
• Does work for arbitrary webviews
• A manual preview refresh will reload resources from virtual file systems if they have an etag.

The disadvantages are:

• Does only refresh images if the webview is reloaded (I guess reloading the entire markdown preview webview is never going to be problematic, so it is fine)
• Resources with etags are not cached anymore with this change. Repeated access to such resources will lead to repeated requests. Most of them will be answered with 304 (not modified) though. If computing the etag is slow, this is going to be problematic.

[mjbvz]

We've had lots of problems in the past around encoding/decoding of special characters in uris. Make sure to add tests for cases where the uri has percent encoded characters

[hediet]

I added some tests with percent characters. I don't know the spec though and what pitfalls there are.

[mjbvz]

Same as above, we may be on a virtual file system so generally we can't assume that we are on a file://.

I think it's fine if you want to restrict the first version of the feature to file uris, but if that's the case, make sure the function names make this clear

[hediet]

This function is now obsolete.
Anyways, as long as the file system watcher does not support virtual file systems (i.e. accepts vscode.Uris instead of the file path based glob), the file:// protocol is the only one that can be supported afaik.

[hediet]

Thanks for your feedback!
I implemented etag-based caching for webview resources and disabled the default caching for resources with etags so that resources with an etag are always refetched.

However, if if-none-match is present and matches the etag of the resource, 304 (not modified) is returned.

The file system watcher is still required to trigger a preview refresh.

[mjbvz]

Thanks! The Etag usage looks much cleaner and should help other webviews as well.

This change should be in the next insiders build