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

Gracefully recover from observer exceptions 🧸 #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZeeJab
Copy link

@ZeeJab ZeeJab commented Dec 9, 2019

Previously, when an exception was thrown no further updates would be able to get scheduled. This PR, adds a try/finally to ensure that the internal state is cleaned up correctly even when an exception gets thrown.

Copy link
Owner

@gpoitch gpoitch left a comment

Choose a reason for hiding this comment

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

I think I am onboard with the need for this now.

Just trying to understand the control flow - as in why a single try around the check call doesn't work. Also consideration for ElementObserver.

Finally, concern if the try deoptimizes the function at all.

@ZeeJab
Copy link
Author

ZeeJab commented Dec 11, 2019

I initially tried to have the try/finally just in the call to check, but PositionObserver has its own state that goes bad if an exception is thrown, which I ran into when writing the test. If you think there's a better way to test PositionObserver that doesn't trigger this behavior, I am open to suggestions.

Regarding deoptimizing the function, my understanding is that this is not much of an issue in modern JavaScript VMs. [Source: my husband]

@gpoitch
Copy link
Owner

gpoitch commented Dec 11, 2019

I tried to make an internal “call” function that simply calls a user callback wrapped in a try but wasn’t continuing for some reason.

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.

2 participants