-
Notifications
You must be signed in to change notification settings - Fork 135
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
refactor: move StreamCreator to types/open-next #663
Conversation
And make onFinish optional
🦋 Changeset detectedLatest commit: 5eb848f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
return Boolean( | ||
this.writableFinished && this.responseStream?.writableFinished, | ||
); | ||
return this.writableFinished && (this.responseStream?.writableFinished ?? true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change in behavior but I believe this is what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the finished()
change. The rest is good
return Boolean( | ||
this.writableFinished && this.responseStream?.writableFinished, | ||
return ( | ||
this.writableFinished && (this.responseStream?.writableFinished ?? true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this one is correct actually.
Your fix is good in case we don't provide a ResponseStream
, but in case we do, we may end up in a situation where OpenNextNodeResponse
is finished, but not this.responseStream
. I'm not 100% sure of that though and not sure what could be the consequences either.
I think we could replace it with
this.responseStream ? this.responseStream.writableFinished : this.writableFinished
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swapped for your suggestion, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks
And make onFinish optional
This is part 1 of 2 in a stack made with GitButler: