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

[Suggestion] Add an onError handler to <collage-fragment> #40

Open
goloroden opened this issue Dec 7, 2023 · 4 comments
Open

[Suggestion] Add an onError handler to <collage-fragment> #40

goloroden opened this issue Dec 7, 2023 · 4 comments
Milestone

Comments

@goloroden
Copy link

The <collage-fragment> web component provides a few lifecycle methods, such as onLoaded. While this is fine, I was wondering if it would be possible to control the behavior in case loading the iframe fails, e.g. because of a network error, or if the targeted server returns a 404.

Would it be possible to add an onError handler, where one might control what should happen in these cases?

@WanjaTschuorSICKAG
Copy link
Collaborator

Hi @goloroden,

I think, this is a very good suggestion. Have you any thoughts about the interface? I think about something like making it configurable with a timeout value or so.

@goloroden
Copy link
Author

Hmmm… 🤔

I think there are multiple cases we need to discuss:

  • It may be that the iframe can not be loaded due to a network issue (e.g., you're offline)
  • It may be that the iframe can not be loaded due to a DNS our routing error (e.g., a typo in the host name)
  • It may be that the iframe can not be loaded because the targeted server returns an unexpected status code (404, 500, …)
  • It may be that the iframe can not be loaded because the server responds super-slowly (which effectively means, we run into a timeout…)

Some of them are obvious (e.g., DNS can't be resolved), some of them are open to interpretation (e.g., what does a 404 actually mean)?

So, maybe we need more than one additional callback. Just to give an idea:

  • One could have onError for cases which are clear. Then onError is called, and the appropriate error is handed over as a parameter
  • One could have an additional response parameter in onLoaded, where one could react to e.g. the status code, if necessary
  • One could also have a timeout property on the component, which defines the duration to wait, and if it passes without the iframe being loaded or an error to be thrown, it then throws a TimeoutError, which in turn triggers the onError callback

Does that make sense?

@WanjaTschuorSICKAG WanjaTschuorSICKAG added this to the v0.3.0 milestone Jan 23, 2024
@kirchsuSICKAG
Copy link
Collaborator

Hi @goloroden.

Sorry for the late response!
It makes absolute sense.
The only thing which I am not sure about is sending an additional response parameter with the status code on the onLoaded. Because for me if the onLoaded is triggered means, everything was fine and therefore only status code 2XX would be a valid status codes for it. Otherwise we maybe could throw an error / trigger the onError callback with the status code other than 2XX.

I like the timeout property. Would it also make sense then to add an retry property, so if it was running into a timeout, we try to connect again automatically? Or is it enough to give the onError + timeout into the hand, so a reconnection try is handled from the user?

We took this feature into our next milestone. But for now we can not say, when we implement it.
If you would like to contribute it, we would be happy. Otherwise thank you very much for your idea, we really like it :)

@goloroden
Copy link
Author

Hey @kirchsuSICKAG,

thanks for the update 😊

Regarding the retry pattern. TBH, I'm not a fan of this, since on the one hand it increases the complexity on your side, and on the other hand it is something that can be done with a few lines by the user. In addition, maybe there is already a retry library in use that one would want to keep using, and then yours would interfere, and so on…

To cut it short: IMHO keep it simple.

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

No branches or pull requests

3 participants