guarantee that return of TreeDataProvider.getChildren() is not mutated by vscode - #306955

I think this is more information than most extension developers will want in the API doc. Instead, could we do something like this?

		getChildren(element?: T): ProviderResult<T[] | readonly T[]>;

That was the original idea, and I would prefer it to be that way.

However, there may be extensions that call getChildren internally and mutate the returned array. While this isn’t the intended usage of TreeDataProvider, it could still exist in practice. If we’re comfortable with those extensions needing to adjust their logic or add casts back to T[] — since TypeScript will surface errors the next time they try to publish — then yes, that works. I assumed this is what you meant by breaking existing extensions.

I suspect such cases are rare (if any), but that’s the only reason I kept ProviderResult<T[]> and added a note instead.

We probably don’t need T[] | readonly T[] in this case? Since readonly dominates, it’s effectively equivalent to:

getChildren(element?: T): ProviderResult<readonly T[]>

After thinking, actually, such implementations which already include return type definition should not suffer, like:

children: Item[];

getChildren(element?: Item): ProviderResult<Item[]> {
    return children;
}

someMethod() {
    this.getChildren().push(newChildren);
}

This is usually done by autocompletion, and after updating interface to getChildren(element?: T): ProviderResult<readonly T[]> this should be ok, since this definition is assignable to it

Here is the only case where typescript will yell at:

children: Item[];

getChildren(element?: Item) { // will need to specify return type here as ProviderResult<Item[]>
    return children;
}

someMethod() {
    this.getChildren().push(newChild); // or add cast here to Item[]
                    // ^^^^ here will be ts error
}

I think, internal usage of this.getChildren() is already rare, and the cases where return type is not specified + output is mutated is close to, or is zero

This probably should be affected too, along with getChildren
onDidChangeTreeData?: Event<T | T[] | undefined | null | void>;
to
onDidChangeTreeData?: Event<T | readonly T[] | undefined | null | void>;

Let me know if this is going to be accepted, I'll update PR then

To keep the possible breakage at zero, let's keep it as you currently have it in the PR.

To keep the possible breakage at zero, let's keep it as you currently have it in the PR.