- Notifications
You must be signed in to change notification settings - Fork928
feature: Load workspace build logs from streaming#1997
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // Watching for the stream | ||
| // eslint-disable-next-line no-constant-condition, @typescript-eslint/no-unnecessary-condition | ||
| while (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.
Do we need to worry about error handling or cleanup ifdone never returns?
BrunoQuaresmaJun 3, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
It is a good point. I think for now we can rely on done being always called, why:
- We are using a polyfill(https://canjs.com/doc/can-ndjson-stream.html) because Firefox does not support some stream APIs
- I took a look and I didn't find anything related to "read failure" or something related
- If an error is thrown, it will stop the execution anyway
- This error, in particular, looks tricky to handle so I would expect someone having issues with that to think of a proper solution
- In summary, I don't know how to do that and it looks complex to handle 😆, so I would not invest too much time on that right now, but if you have an idea how to solve that, I would love to learn and implement it for sure.
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.
Ha! Thanks for the breakdown - what you said makes sense and I think you're right to hold off. If this becomes an issue in the future, we can look intocallingcancel on the ReadableStream object.
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.
Ahh, nice!
| return ( | ||
| <div key={stage}> | ||
| <div className={styles.header}> | ||
| <div>{stage}</div> | ||
| {duration && <div className={styles.duration}>{duration} seconds</div>} | ||
| {shouldDisplaySpinner && <CircularProgress size={14} className={styles.spinner} />} | ||
| {shouldDisplayDuration && <div className={styles.duration}>{duration} seconds</div>} |
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.
should we put "seconds" in aLanguage block?
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.
Definetely!
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.
Awesome!
Uh oh!
There was an error while loading.Please reload this page.
Closes#1769