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

Fix Alpine.js becoming unresponsive after uncaught exception in reactive effect callback #3279

Conversation

ferranconde
Copy link
Contributor

@ferranconde ferranconde commented Nov 22, 2022

Fix Alpine.js unresponsive after uncaught exception in reactive effect callback

Context

Alpine.js executes reactive effects when the application state changes. Some of these reactive effects involve executing
custom methods defined by the application developers (e.g. methods for x-show, x-text) and thus may contain bugs.
When one of these callbacks raises an unexpected exception, the whole application becomes unresponsive because Alpine does not handle this gracefully.

In the following reproducible example (https://codepen.io/ferranconde/pen/PoaQJaX?editors=1010),
there's a component whose x-show method features a faulty implementation.
After clicking on the orange button, any other component, e.g. the counter text + button, stops working: the application is in an unrecoverable state.

The issue is that, in scheduler.js:flushJobs(), if there is an uncaught exception in the for loop that traverses
the job queue, the flushing flag is not properly reset. On any subsequent state changes, the condition for queueFlush()
to call flushJobs will never be true anymore. A page reload is then needed.

Fix

As per this comment, the place where this issue should be solved is in the evaluator. When the custom method for x-show, x-text... is defined as a method, Alpine's custom error handler is not being applied; it's only applied when the custom method is defined as a string.
The proposed change avoids returning early when expression is of typeof function, thus ensuring that the error handler applies to this case as well.

…f a job callback executes a faulty implementation where an uncaught exception is raised.
@SimoTod
Copy link
Collaborator

SimoTod commented Nov 26, 2022

I believe the real issue is in the evaluator. There is a custom error handler in alpine which prints a useful debug warning in the console and keep the stack running. It seems like is not applied in all cases.
See https://github.com/alpinejs/alpine/blob/main/packages/alpinejs/src/evaluator.js#L42-L48

The generateEvaluatorFromFunction (which runs in your example) doesn't use the ad hoc handler.

I suppose the code at those line it should really be something like

evaluator = (typeof expression === 'function') 
  ? generateEvaluatorFromFunction(dataStack, expression)
  : generateEvaluatorFromString(dataStack, expression, el);

return tryCatch.bind(null, el, expression, evaluator)

Ferran Conde Codorniu added 2 commits December 2, 2022 17:02
…t even if a job callback executes a faulty implementation where an uncaught exception is raised."

This reverts commit 32130fe.
…nction, as well as generateEvaluatorFromString.
@ferranconde
Copy link
Contributor Author

@SimoTod thanks for the pointer to the root cause. I've been able to confirm that your proposed change indeed fixes the issue. I've updated the PR to reflect this.

@joshhanley
Copy link
Collaborator

@ferranconde thanks for the PR! Could you add a test for this? Thanks!

@calebporzio calebporzio merged commit 402be31 into alpinejs:main Jan 27, 2023
@calebporzio
Copy link
Collaborator

Looks good to me. I'm not sure why the direct function evaluator wasn't put through the try catch as well.

Hopefully not for some specific reason we're about to find out when we tag the next release lol.

Thanks for digging deep on this problem @ferranconde and @SimoTod!

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.

None yet

4 participants