Skip to content

Conversation

calebcordry
Copy link
Member

Takes a given response and writes it to a given writer. To be used for ad streaming.

Part of #27189

* @param {!WritableStreamDefaultWriter} writer
* @return {!Promise}
*/
export function streamResponseToWriter(win, response, writer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this used? could you add it to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, was trying to keep it simple but can make a larger pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

was wondering how this method is going to be used. you can still leave TODOs in the caller to make the PR not too large.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the calling context.

</html>
`;

describes.fakeWin('streamResponseToWriter', {}, (env) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we want to handle an interrupted stream?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean if the browser cancels the response? I'm not sure, but if that is the same as calling .cancel on the stream, the writer will be closed and if there is no content we will collapse the ad. If there is a body I think we will render a partial ad. If there is no cancel called I think we will just wait until we receive a body, though maybe we should time out. Not sure I am answering your question, let me know if you want to chat about this.

Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

This looks good. My only comment is that this could easily to into src/utils/.

@calebcordry
Copy link
Member Author

Moved util to src/utils

@calebcordry
Copy link
Member Author

@dvoytenko friendly ping :) I need your approval now that the new files live in src/

@calebcordry calebcordry merged commit 9bebe0a into ampproject:master Jun 17, 2020
@calebcordry calebcordry deleted the response-stream branch June 17, 2020 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants