fix: corrected the disconnection behavior to get stream logs Close #1…

[angeliski]

Hey, I just made a Pull Request!

Hey team, I put a fix for #15002

The problem is described at the issue but the basic is:
We receive a disconnection (internet issues) and don't get any feedback (or update) after that. So I did two things:

1. Change the empty message to We cannot connect at the moment, trying again in some seconds... Retrying (3/3 retries)
   
2. Put a simple retry after 15s using a timeout. In my case I didn't get the backend running again, so I got a Failed to fetch message

I tried to change the error behavior from the observable (resulting in the subscriber disconnection). Still, I got a very messy code, so this is a simple enough solution.

✔️ Checklist

• A changeset describing the change and affected packages. (more info)
• Added or updated documentation
• [N/A] Tests for new functionality and regression tests for bug fixes
• Screenshots attached (for UI changes)
• All your commits have a Signed-off-by line in the message. (more info)

[backstage-goalie]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/plugin-scaffolder-react	plugins/scaffolder-react	patch	v1.4.0

[github-actions]

Uffizzi Preview deployment-25612 was deleted.

[jhaals]

Thanks for the PR!

A minor note/question regarding the retry that is forever now if I read the code correctly.
Would it be terrible to add some tests to this hook/functionality to gain some confidence for further refactors?

Would love for @benjdlambert to take a look at this as well.

[jhaals]

Thanks for the PR!

A minor note/question regarding the retry that is forever now if I read the code correctly.
Would it be terrible to add some tests to this hook/functionality to gain some confidence for further refactors?

Would love for @benjdlambert to take a look at this as well.

[benjdlambert]

Interestingly what is the error type at this point and why does it not have a message?

[angeliski]

I just was getting an ERR_NETWORK_CHANGED 200 in the browser, but didn't find a simple way to catch this error in the code, so I used the empty message to change this.

Do you have suggestions on how to catch this?

[benjdlambert]

Hmm - I wonder if these are just browser errors and are not available in javascript though. If you stringify the error that you get in the useEventStream and log it what does it look like?

[angeliski]

It is an event from the EventSource, so just generic information 🤔

[benjdlambert]

ok let's go with this

[benjdlambert]

We could really do with tests for this too, but you don't have to do that in this PR.

[jhaals]

Please make sure to run prettier for the build to pass :)

[angeliski]

Fixed @jhaals, sorry for this taking so long

@benjdlambert I didn't find a test for the useEventStream hook, Do you expect a test for the whole file or a test in another place?
I will to raise test for this soon

[jhaals]

@benjdlambert I didn't find a test for the useEventStream hook, Do you expect a test for the whole file or a test in another place? I will to raise test for this soon

I think it make sense to add a new test for the useEventStream file and add some cases for this :) Would be lovely as a followup PR 🙏

[jhaals]

@angeliski if you rebase this PR against master the build should go green 🤞

[benjdlambert]

Our e2e's are b0rked right now because of another octokit issue.

[github-actions]

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.15.0 release, scheduled for Tue, 20 Jun 2023.

[benjdlambert]

But merged this anyways 🙏 Thanks @angeliski