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 x-if sub expressions still being evaluated after x-if evals to false #2556

Merged
merged 4 commits into from Jan 21, 2022

Conversation

kylethielk
Copy link
Contributor

Hi There!

This PR is to address an issue where after an <template x-if="expression"></template> has evaluated to false and thus the DOM element has been removed, sub expressions are still evaluated. Example reports of this issue are: #2540 #2296 #1837 #1917

Imagine the following code:

<div x-data="{user: {name: 'lebowski'}}">
  <button @click="user=null">Logout</logout>
  <template x-if="user">
    <span x-text="user.name"></span>
  </template>
</div>

Before the suggested fix the above code would produce the following when "Logout" is clicked:

Screen Shot 2022-01-03 at 5 03 05 PM

This is because of the following pseudo timeline:

  1. user is set to null
  2. This causes effect() on the x-text directive to queue an expression evaluation i.e user.name.
  3. The x-if="user" expression is evaluated and the result is now false. The cleanup tasks run and removes the <span></span> element from the DOM.
  4. The queue runs and evaluates the expression from Step 2. At this point user is null and thus user.name throws an exception.

My solution is to simply remove the queued effect from the queue in the el._x_undoIf() method call. I am however not sure how maintainers will feel about exposing a new method in scheduler.js though. I could not determine a better way though.

This was my first peek into the internals of Alpine so there very well may be a better way to solve this issue. I added a new Cypress test to demonstrate this bug and to also demonstrate how this solution fixes the bug.

Very open to feedback on a better solution.

@kylethielk
Copy link
Contributor Author

I've pushed a new commit 2dc402b to extend this fix for x-for as well thanks to comment by @SimoTod here.

Copy link
Contributor

@markjaquith markjaquith left a comment

Choose a reason for hiding this comment

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

This is working great, and fixes a seriously annoying bug. Nicely done. 👏🏻 Ran my own tests, and this definitely fixes the issue.

All I have is a bunch of boring code formatting consistency advice.

packages/alpinejs/src/directives/x-for.js Outdated Show resolved Hide resolved
packages/alpinejs/src/directives/x-for.js Outdated Show resolved Hide resolved
packages/alpinejs/src/directives/x-if.js Outdated Show resolved Hide resolved
packages/alpinejs/src/directives/x-if.js Outdated Show resolved Hide resolved
packages/alpinejs/src/scheduler.js Outdated Show resolved Hide resolved
@kylethielk
Copy link
Contributor Author

Thanks @markjaquith . Pushed recommended changes.

@calebporzio
Copy link
Collaborator

This looks great. Thanks for contributing!

@calebporzio calebporzio merged commit d49a0b0 into alpinejs:main Jan 21, 2022
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

3 participants