Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

End asyncStream later in the process (before piping) #185

Merged
merged 2 commits into from
Sep 22, 2017

Conversation

DeTeam
Copy link
Contributor

@DeTeam DeTeam commented Sep 21, 2017

Right now asyncStream ends as soon as we stop pushing things to the stringifier stream, which means we can't push things to asyncStream from our customer tags in handleTag.

To support async fragments there we need to end asyncStream later. It seem like proper place would be to end it right before piping it into the resulting stream. I could either do it in the stringifier stream, but though that it'd be better to do via events (plugged event).

@codecov
Copy link

codecov bot commented Sep 21, 2017

Codecov Report

Merging #185 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   98.28%   98.29%   +0.01%     
==========================================
  Files          14       14              
  Lines         582      586       +4     
  Branches      103      104       +1     
==========================================
+ Hits          572      576       +4     
  Misses         10       10
Impacted Files Coverage Δ
lib/streams/stringifier-stream.js 100% <100%> (ø) ⬆️
lib/request-handler.js 99.05% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eba01e...7d91770. Read the comment docs.

@@ -22,6 +22,8 @@ module.exports = class StringifierStream extends stream.Transform {
}
let st = this.queue.shift();
if (st instanceof stream) {
st.emit('plugged');
Copy link
Collaborator

Choose a reason for hiding this comment

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

would only emit this if its a instance of async stream though. Totally useless for other fragment streams.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently yes, but I didn't want to mix up knowledge of async stream into the stringifier stream

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case we would need alternative way of handling for this particular instance.

@vigneshshanmugam
Copy link
Collaborator

Also could you add a test with the issue so that its easier to track why we moved re-emit it again.

@DeTeam
Copy link
Contributor Author

DeTeam commented Sep 21, 2017

Sure thing, on it!

st.end({
name: 'fragment',
attributes: {
async: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, since the fragment's async + also located in handleTag, there would be an error, cause one can't write to async channel after it's ended (which happens on 'finish' events which is all fine, cause we stop writing to the original resultStream right after pushing all parsed chunks).

@DeTeam
Copy link
Contributor Author

DeTeam commented Sep 22, 2017

👍

@vigneshshanmugam
Copy link
Collaborator

👍

@vigneshshanmugam vigneshshanmugam merged commit b253598 into master Sep 22, 2017
@vigneshshanmugam vigneshshanmugam deleted the end-async-later branch September 22, 2017 09:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants