Skip to content
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

Support streaming output #84

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lyubortk
Copy link

No description provided.

src/executable-code/executable-fragment.js Show resolved Hide resolved
src/executable-code/executable-fragment.js Outdated Show resolved Hide resolved
src/webdemo-api.js Show resolved Hide resolved
src/webdemo-api.js Show resolved Hide resolved
package.json Show resolved Hide resolved
src/executable-code/executable-fragment.js Outdated Show resolved Hide resolved
src/view/output-view.js Outdated Show resolved Hide resolved
@AlexanderPrendota
Copy link
Collaborator

@lyubortk @zoobestik Let's use streaming if the attribute stream or async are presented

Copy link
Contributor

@zoobestik zoobestik left a comment

Choose a reason for hiding this comment

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

@lyubortk look, pls. I have some questions.

src/executable-code/executable-fragment.js Outdated Show resolved Hide resolved
src/executable-code/executable-fragment.js Outdated Show resolved Hide resolved
applyStateUpdate(stateUpdate) {
if (stateUpdate.errors === null) {
stateUpdate.errors = []
} else if (stateUpdate.errors !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do it after filter and code should be simpler after that.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I placed this peace of code after filtration but could not come up with any idea how to make it simpler. We must distinguish null here because passing null as stateUpdate.errors tells the function to remove previous errors in the state. Therefore if is necessary here.

src/executable-code/executable-fragment.js Show resolved Hide resolved
`Please click <a href=http://kotl.in/issue target=_blank>here<a> to submit it ` +
const BUG_FLAG = 'BUG'
const BUG_REPORT_MESSAGE = 'Hey! It seems you just found a bug! \uD83D\uDC1E\n' +
`Please go here -> http://kotl.in/issue to submit it ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

What is happened with link? :(

Copy link
Author

Choose a reason for hiding this comment

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

Merging DOM nodes turns into text all inner elements so the link will not be clickable.
I thought that this case with BUG_FLAG is too rare and not really important to add specific behaviour for it.
Is it possible to merge nodes without destroying children?

return `<div class="test-time">Total test time: ${testResults.totalTime}s</div>`
}

export function processJUnitTestResult(testRunInfo, testResults) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@lyubortk actually i don't understand what is happend with test output. Can you explain me differences in this output. It's too much changes not clear for me :(

Copy link
Author

@lyubortk lyubortk May 23, 2020

Choose a reason for hiding this comment

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

I had to rewrite that because each test result must be handled separately with streaming.

src/webdemo-api.js Outdated Show resolved Hide resolved
src/webdemo-api.js Outdated Show resolved Hide resolved
@zoobestik
Copy link
Contributor

zoobestik commented May 22, 2020

@AlexanderPrendota i'm still don't understand why do we need async attribute? :(

@lyubortk
Copy link
Author

I returned synchronous mode back and added asyncServer parameter. Could you please validate that this is the right way of introducing new parameters?

@lyubortk
Copy link
Author

I also added some handling of HTTP error codes and failed requests for streaming (I print a message as error-output to the console if something goes wrong).
But the synchronous mode still does not tell users about errors :(.
I can print errors for synchronous mode too if you want.

README.md Outdated Show resolved Hide resolved
@lyubortk lyubortk requested a review from zoobestik June 7, 2020 22:16
@nikpachoo nikpachoo removed the request for review from zoobestik June 16, 2021 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants