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

Don't use findTreeItem in AzExtTreeFileSystem #1257

Merged
merged 5 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions utils/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1435,10 +1435,15 @@ export type AzExtItemUriParts = {
query: AzExtItemQuery;
};

export interface AzExtTreeFileSystemItem {
id: string;
refresh?(context: IActionContext): Promise<void>;
}

/**
* A virtual file system based around AzExTreeItems that only supports viewing/editing single files.
* A virtual file system based around {@link AzExtTreeFileSystemItem} that only supports viewing/editing single files.
*/
export declare abstract class AzExtTreeFileSystem<TItem extends AzExtTreeItem> implements FileSystemProvider {
export declare abstract class AzExtTreeFileSystem<TItem extends AzExtTreeFileSystemItem> implements FileSystemProvider {
public abstract scheme: string;

public constructor(tree: AzExtTreeDataProvider);
Expand Down
39 changes: 16 additions & 23 deletions utils/src/AzExtTreeFileSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,20 @@ import { Disposable, Event, EventEmitter, FileChangeEvent, FileStat, FileSystemE
import * as types from '../index';
import { callWithTelemetryAndErrorHandling } from "./callWithTelemetryAndErrorHandling";
import { localize } from "./localize";
import { AzExtTreeDataProvider } from "./tree/AzExtTreeDataProvider";
import { AzExtTreeItem } from "./tree/AzExtTreeItem";
import { nonNullProp } from "./utils/nonNull";

const unsupportedError: Error = new Error(localize('notSupported', 'This operation is not supported.'));

export abstract class AzExtTreeFileSystem<TItem extends AzExtTreeItem> implements FileSystemProvider {
export abstract class AzExtTreeFileSystem<TItem extends types.AzExtTreeFileSystemItem> implements FileSystemProvider {

private readonly itemCache: Map<string, TItem> = new Map<string, TItem>();
Copy link
Member

Choose a reason for hiding this comment

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

Should we clear and repopulate this every time a refresh happens? Seems like it could cause stale tree item ids if we're not careful.

Copy link
Member Author

@alexweininger alexweininger Dec 20, 2022

Choose a reason for hiding this comment

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

I haven't run into that in my testing, but I am worried about the possibility of something like that happening. The tree item ids correspond to Azure resource ids so unless something is deleted I think it's fine. I will test that now. I don't know if delete was handled properly in v1.

If you delete a resource while editing the tags it will show this error. I think that's fine behavior.
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Also whenever I tried clearing this map, it resulted in errors since we can't really know if the items in the map are still in use easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

A potential fix for all of this would be to really remove the reliance on items at all, and base if fully on URIs.


public abstract scheme: string;

private readonly _emitter: EventEmitter<FileChangeEvent[]> = new EventEmitter<FileChangeEvent[]>();
private readonly _bufferedEvents: FileChangeEvent[] = [];
private _fireSoonHandle?: NodeJS.Timer;

private _tree: AzExtTreeDataProvider;

public constructor(tree: AzExtTreeDataProvider) {
this._tree = tree;
}

public get onDidChangeFile(): Event<FileChangeEvent[]> {
return this._emitter.event;
}
Expand All @@ -37,7 +32,9 @@ export abstract class AzExtTreeFileSystem<TItem extends AzExtTreeItem> implement
public abstract getFilePath(item: TItem): string;

public async showTextDocument(item: TItem, options?: TextDocumentShowOptions): Promise<void> {
await window.showTextDocument(this.getUriFromItem(item), options);
const uri = this.getUriFromItem(item);
this.itemCache.set(item.id, item);
await window.showTextDocument(uri, options);
}

public watch(): Disposable {
Expand Down Expand Up @@ -70,7 +67,7 @@ export abstract class AzExtTreeFileSystem<TItem extends AzExtTreeItem> implement
await callWithTelemetryAndErrorHandling('writeFile', async (context) => {
const item: TItem = await this.lookup(context, uri);
await this.writeFileImpl(context, item, content, uri);
await item.refresh(context);
await item.refresh?.(context);
});
}

Expand Down Expand Up @@ -115,28 +112,24 @@ export abstract class AzExtTreeFileSystem<TItem extends AzExtTreeItem> implement
);
}

protected getUriParts(item: TItem): types.AzExtItemUriParts {
return {
protected findItem(query: types.AzExtItemQuery): TItem | undefined {
return this.itemCache.get(query.id);
}

private getUriFromItem(item: TItem): Uri {
const data: types.AzExtItemUriParts = {
filePath: this.getFilePath(item),
query: {
id: item.fullId
id: item.id
}
};
}

protected async findItem(context: types.IActionContext, query: types.AzExtItemQuery): Promise<TItem | undefined> {
return await this._tree.findTreeItem(query.id, context);
}

private getUriFromItem(item: TItem): Uri {
const data: types.AzExtItemUriParts = this.getUriParts(item);
const query: string = stringifyQuery(data.query);
const filePath: string = encodeURIComponent(data.filePath);
return Uri.parse(`${this.scheme}:///${filePath}?${query}`);
}

private async lookup(context: types.IActionContext, uri: Uri): Promise<TItem> {
const item: TItem | undefined = await this.findItem(context, this.getQueryFromUri(uri));
const item: TItem | undefined = this.findItem(this.getQueryFromUri(uri));
if (!item) {
context.telemetry.suppressAll = true;
context.errorHandling.rethrow = true;
Expand Down