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

Retry to resolve an element #81

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Retry to resolve an element #81

merged 4 commits into from
Jul 8, 2021

Conversation

julianrubisch
Copy link
Contributor

Enhancement

Description

Wrapped the dispatchAppearEvent(entry, observer) in a setInterval, which is cleared upon disconnection. Note that there is a bit of a shotgun surgery here, which at the moment I don't know to avoid - but it's only 3 instances.

Also yet TODO is the configuration of the interval time. I suggest specifying this via a custom element attribute, which could be passed in by the helper.

Fixes #75

Why should this be added

Resilience to network conditions etc.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@julianrubisch
Copy link
Contributor Author

@rickychilcott care to weigh in? I think this should do it...

@rickychilcott
Copy link
Contributor

I've got this pulled up and will take a look in the morning.

@julianrubisch
Copy link
Contributor Author

we could also try exponential backoff like in rails/rails@cc9a9e8

thanks @leastbad for the suggestion!

@rickychilcott
Copy link
Contributor

I'm sorry for failing to review this. It seems like a very reasonable solution and doesn't feel like it too invasive of a solution.

RE: exponential backoff. It's a good point. Perhaps 3 seconds is enough for a small partial, but it could be a massive partial that is really taking longer to generate, and continuing to hammer every X seconds would be counterproductive.

It would significantly complicate the code, but as you link to or perhaps this implementation it might not be too bad.

Is there anything I can do to wrap this up? I now have approval to start implementing SR into my application slowly, and futurism is going to be a big part of performing the perceived performance of the application. This will be quite helpful.

@leastbad
Copy link
Contributor

That's great news, Ricky!

@julianrubisch
Copy link
Contributor Author

julianrubisch commented Apr 19, 2021

Thank you @rickychilcott ! I will give your link a thorough read. I hope that with exponential backoff we can craft a config-less solution.

Will revisit this later this week.

@rickychilcott
Copy link
Contributor

👏 👏 👏

This looks good!

@julianrubisch
Copy link
Contributor Author

Cool will cross check tomorrow and merge

@julianrubisch julianrubisch marked this pull request as ready for review July 6, 2021 09:24
@julianrubisch julianrubisch merged commit befee43 into master Jul 8, 2021
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.

Retry
3 participants