Skip to content

Commit

Permalink
Ensure addActivityItemStream() doesn't lose Output vs Error str…
Browse files Browse the repository at this point in the history
…eam specialization (#4848)

Addresses #4519

In `addActivityItemStream()`, either a `ActivityItemOutputStream` or
`ActivityItemErrorStream` always goes in, but it was possible for a
"generic" `ActivityItemStream` to come out here when we make one from
the `remainderText` after the last newline:


https://github.com/posit-dev/positron/blob/e51332d5baa59b72f5e8b28861bbb751f25452ea/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts#L118-L136

That generic `ActivityItemStream` would get set here:

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L110-L116

And then pushed onto `this._activityItems`

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L138-L139

The problem is that our Console code doesn't know how to deal with a
generic `ActivityItemStream`. It must be a specialized
`ActivityItemOutputStream` or `ActivityItemErrorStream`!

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/contrib/positronConsole/browser/components/runtimeActivity.tsx#L46-L49

If a generic `ActivityItemStream` slipped through, the output would
never appear in the Console, making it look "dropped", as reported in
#4519.

---

I like how we have a generic `ActivityItemStream` class that we
specialize for the output/error types, so I've fixed this bug by using
[polymorphic
`this`](https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types)
to allow `addActivityItemStream()` to take and return objects of the
correct specialized type.

Ideally we'd simply be able to use `new this.constructor()` in place of
`new ActivityItemStream()` to use the polymorphic constructor when we
need to create new objects, but due to some [well known
weirdness](microsoft/TypeScript#3841) in
typescript, you have to manually cast it to the constructor type, so
`newActivityItemStream()` wraps that up.

I've also made the `ActivityItemStream` constructor `protected` to avoid
us using it by accident in the codebase. We should _always_ be creating
`ActivityItemOutputStream` or `ActivityItemErrorStream` instead.

Before 😢 



https://github.com/user-attachments/assets/b1854d9e-94eb-4952-aead-bebcf5c5d6a2



After 🥳 


https://github.com/user-attachments/assets/4a76974c-6862-4e84-a296-620717ddb8da
  • Loading branch information
DavisVaughan committed Sep 30, 2024
1 parent e51332d commit 659e073
Showing 1 changed file with 50 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export class ActivityItemStream {
/**
* Gets the ActivityItemStream array.
*/
private activityItemStreams: ActivityItemStream[] = [];
private activityItemStreams: this[] = [];

/**
* Gets the ANSIOutput that is processing this ActivityItemStream.
Expand Down Expand Up @@ -47,12 +47,17 @@ export class ActivityItemStream {

/**
* Constructor.
*
* Never to be called directly.
* Internally, use `newActivityItemStream()` instead.
* Externally, use `ActivityItemOutputStream` or `ActivityItemErrorStream` constructors instead.
*
* @param id The identifier.
* @param parentId The parent identifier.
* @param when The date.
* @param text The text.
*/
constructor(
protected constructor(
readonly id: string,
readonly parentId: string,
readonly when: Date,
Expand All @@ -70,9 +75,7 @@ export class ActivityItemStream {
* @param activityItemStream The ActivityItemStream to add.
* @returns The remainder ActivityItemStream, or undefined.
*/
public addActivityItemStream(
activityItemStream: ActivityItemStream
): ActivityItemStream | undefined {
public addActivityItemStream(activityItemStream: this): this | undefined {
// If this ActivityItemStream is terminated, copy its styles to the ActivityItemStream being
// added and return it as the remainder ActivityItemStream to be processed.
if (this.terminated) {
Expand All @@ -95,7 +98,7 @@ export class ActivityItemStream {
const remainderText = activityItemStream.text.substring(newlineIndex + 1);

// Add an ActivityItemStream with the text containing the newline.
this.activityItemStreams.push(new ActivityItemStream(
this.activityItemStreams.push(this.newActivityItemStream(
activityItemStream.id,
activityItemStream.parentId,
activityItemStream.when,
Expand All @@ -116,7 +119,7 @@ export class ActivityItemStream {
}

// Create the remainder ActivityItemStream.
activityItemStream = new ActivityItemStream(
activityItemStream = this.newActivityItemStream(
activityItemStream.id,
activityItemStream.parentId,
activityItemStream.when,
Expand Down Expand Up @@ -153,15 +156,53 @@ export class ActivityItemStream {
}
}

/**
* Polymorphic constructor for internal creation of new `ActivityItemStream`s
*
* Uses polymorphic `this` to actually return extension class types, like
* `ActivityItemOutputStream` and `ActivityItemErrorStream`.
*
* Note that we have to manually cast `this.constructor()` to the right type, as otherwise
* it is just a generic `Function`.
* https://github.com/microsoft/TypeScript/issues/3841
* https://stackoverflow.com/questions/64638771/how-can-i-create-a-new-instance-of-a-class-using-this-from-within-method
*
* @param id The identifier.
* @param parentId The parent identifier.
* @param when The date.
* @param text The text.
* @returns A newly constructed activity item stream of type `this`.
*/
private newActivityItemStream(
id: string,
parentId: string,
when: Date,
text: string
): this {
const constructor = (
this.constructor as
new (id: string, parentId: string, when: Date, text: string) => this
);
return new constructor(id, parentId, when, text);
}

//#endregion Private Methods
}

/**
* ActivityItemOutputStream class.
*/
export class ActivityItemOutputStream extends ActivityItemStream { }
export class ActivityItemOutputStream extends ActivityItemStream {
constructor(id: string, parentId: string, when: Date, text: string) {
super(id, parentId, when, text);
}
}

/**
* ActivityItemErrorStream class.
*/
export class ActivityItemErrorStream extends ActivityItemStream { }
export class ActivityItemErrorStream extends ActivityItemStream {
constructor(id: string, parentId: string, when: Date, text: string) {
super(id, parentId, when, text);
}
}

0 comments on commit 659e073

Please sign in to comment.