Alpine: x-bind:class is removing classes

[bep]

See https://codepen.io/bepsays/pen/xxwwzVP

Ref. the documentation

For classes, you pass in an object who's keys are class names, and values are boolean expressions to determine if those class names are applied or not.

So, given this:

<div x-data="{ open: true, level: 32 }">
  <style>
    .red {
      background-color: red;
    }
    .blue {
      background-color: blue;
    }

    .text-yellow {
      color: yellow;
    }

    .text-blue {
      color: green;
    }


  </style>
  <button x-bind:class="{ 
                          'red text-yellow' : open,
                          'red text-blue' : !open && level === 12,
                        }">Button</button>

</div>

I would expect a red button with yellow text. But the red class is removed by the second statement.

I now notice that if I switch the conditions above, I get the colors I want, but that is for this very simple case.

Is there a reason why it's implemented like this? Is this how Vue does it?

[HugoDF]

At face value this sounds like a bug, good catch!.

I think the logic is a bit naive and just deleting all the old classes instead of merging all active classes.

I'll have another look later and confirm/correct the above statement.

[bep]

I think the logic is a bit naive and just deleting all the old classes instead of merging all active classes.

I don't think it's removing all the old classes (that would certainly be a bug), but it toggles the classes (add or remove) in the list depending on the expression.

[muzafferdede]

here is the piece of the code that handles the class binding

...
} else if (attrName === 'class') {
        if (Array.isArray(value)) {
            const originalClasses = el.__x_original_classes || []
            el.setAttribute('class', arrayUnique(originalClasses.concat(value)).join(' '))
        } else if (typeof value === 'object') {
            Object.keys(value).forEach(classNames => {
                if (value[classNames]) {
                    classNames.split(' ').forEach(className => el.classList.add(className))
                } else {
                    classNames.split(' ').forEach(className => el.classList.remove(className))
                }
            })
        } else {
            const originalClasses = el.__x_original_classes || []
            const newClasses = value.split(' ')
            el.setAttribute('class', arrayUnique(originalClasses.concat(newClasses)).join(' '))
        }
    }

Logic has issue. i think it should check classes as group instead of individual class for binding and unbinding.

            Object.keys(value).forEach(classNames => {
                if (value[classNames]) {
                    classNames.split(' ').forEach(className => el.classList.add(className))
                } else {
                    classNames.split(' ').forEach(className => el.classList.remove(className))
                }
            })

[SimoTod]

@HugoDF @muzafferdede If you fix this issue, it would also be good to address #156.
I think the all class logic can be improved.

[muzafferdede]

just tried a logic and i think it worked but not sure. Maybe you guys can take a look.

...
            // Store freshly added classes to prevent unexpected removing
            let freshClasses = []
            Object.keys(value).forEach(classNames => {
                if (value[classNames]) {
                    classNames.split(' ').forEach(className => (el.classList.add(className),
                        freshClasses.push(className)))
                } else {
                    classNames.split(' ').forEach(className => {
                        // Check if we just added the class
                        if (!freshClasses.includes(className)) {
                            el.classList.remove(className)
                        }
                    })
                }
            })
...

i bind freshly added classes to a freshClasses variable and when unbinding classes, i check if we just added that class, if so, keep it, if not then remove it. Not sure if that's all or i am missing something tho.

update
Tests are green. Forgot to mention

[SimoTod]

@muzafferdede does it work the second time the state changes?

Simple example with just one class.

<div x-data="{highlight: true}">
  <p x-bind:class="{yellow: highlight}">
    Test
  </p>
  <button @click="highlight = false">OFF</button>
  <button @click="highlight = true">ON</button>
</div>

I'm under the impression that the code won't remove the highlight class since it's not in the fresh class list, right?

We also need to write a test to check this specific case, if possible.

[SimoTod]

A possible approach would be to add classes to a remove and add array instead of manipulating them in the loop. After the loop, you can then process all the removals first, then the additions. In that way, you'll make sure that all the classes which are not supposed to be there are removed and then you add all the classes which evaluate to true as per documentation.

[muzafferdede]

hmm.. i am middle of A Chicken and Egg Situation. Let me ask you, what would you expect in this case:

    <div x-data="{highlight: true}">
        <p x-bind:class="{'yellow': highlight, 'yellow': !highlight}">
            Test
        </p>
    </div>

will the p tag end up with yellow class or not?

[bep]

will the p tag end up with yellow class or not?

Orange ... :-) I would expect yellow.

As in: I'm not sure why classes gets removed. I think this should be made much simpler: If expression evaluates to true, then add the classes if not already there.

For classes, you pass in an object who's keys are class names, and values are boolean expressions to determine if those class names are applied or not.

"applied or not"; it doesn't say anything about removing them.

[ryangjchandler]

will the p tag end up with yellow class or not?

Orange ... :-) I would expect yellow.

As in: I'm not sure why classes gets removed. I think this should be made much simpler: If expression evaluates to true, then add the classes if not already there.

For classes, you pass in an object who's keys are class names, and values are boolean expressions to determine if those class names are applied or not.

"applied or not"; it doesn't say anything about removing them.

I agree with you on this one. They should be merged instead of removed then added.

[SimoTod]

Yellow but only because the documentation is written in this way.

@bep the reason why classes are removed is that, when your state changes, you want to remove any classes you added on the previous iteration because at that point they are already in the DOM (e.g. When open changes).

But yeah, another approach would be to reset the class attribute to the original list at the start of each refresh and only add those which evaluate to true

[bep]

But yeah, another approach would be to reset the class attribute to the original list at the start of each refresh and only add those which evaluate to true

Yes!

[muzafferdede]

so const originalClasses = el.__x_original_classes || [] merge with classes that evaluate to true? ignore any other classes?

[SimoTod]

It would be worth to check how Vue behave as well (I'm not precious about that but I think Caleb prefers to stick to what Vue does).

[ryangjchandler]

Vue will maintain the original class list and only add those that are true. See this pen: https://codepen.io/ryangjchandler/pen/zYvvMVg Click the button to see the changes.

I think the entire bind.js file could do with a tidy up since it's quite hard to follow at first glance.

[muzafferdede]

i think this is it then

         } else if (typeof value === 'object') {
            let classesToAdd = el.__x_original_classes || []
            Object.keys(value).forEach(classNames => {
                if (value[classNames]) {
                    classNames.split(' ').forEach(className => classesToAdd.push(className))
                }
                classesToAdd.forEach(className => el.classList.add(className))
            })
        }

minus
class attribute bindings are removed by object syntax
test fails since there is no more removing classes

[ryangjchandler]

i think this is it then

         } else if (typeof value === 'object') {
            let classesToAdd = el.__x_original_classes || []
            Object.keys(value).forEach(classNames => {
                if (value[classNames]) {
                    classNames.split(' ').forEach(className => classesToAdd.push(className))
                }
                classesToAdd.forEach(className => el.classList.add(className))
            })
        }

minus
class attribute bindings are removed by object syntax
test fails since there is no more removing classes

I've got a passing PR ready to submit :)

[ryangjchandler]

I've submitted a PR (linked above). The implementation is different from what we originally said in this thread, since classes aren't always being added so we can't always overwrite the class attribute.

In some cases, a class that is already on the element prior to Alpine starting might need to be removed due to the binding conditions. I've updated the code so that any bindings that evaluate to false, in @bep's case the red text-blue binding, will be run before any new classes are added. This will ensure that classes that occur in both bindings, red text-yellow and red text-blue (red)` will be persisted across each render.

The footprint of the method is nearly the same. I did consider using el.classList.toggle('class', value[classNames]) instead of the if statements but the performance gain / code cleanliness isn't worth it imo.

[bep]

In some cases, a class that is already on the element prior to Alpine starting might need to be removed due to the binding conditions

That does not sound right to me. If I define <div class="foo"> then I don't want Alpine to ... ever remove class foo.

[ryangjchandler]

In some cases, a class that is already on the element prior to Alpine starting might need to be removed due to the binding conditions

That does not sound right to me. If I define <div class="foo"> then I don't want Alpine to ... ever remove class foo.

Yeah, it seems a bit strange but it was already a feature prior to my PR, it also has a test in bind.spec.js labelled class attribute bindings are removed by object syntax (on mobile so can't link to file & line) on line 79.

I guess you might want to remove a class after fetching some data via fetch() or similar. Would need to ask Caleb though.

[bep]

I guess you might want to remove a class after fetching some data via fetch() or similar.

You can certainly do that with the solution we agreed was the correct one. I suggest you base the PR on the implementation outlined above (which 4 persons have thumbed up) and not some old assumption that didn't exactly work without fault on the first implementation of this. Ambiguity is a prime source for new bugs/confusion.

[ryangjchandler]

I guess you might want to remove a class after fetching some data via fetch() or similar.

You can certainly do that with the solution we agreed was the correct one. I suggest you base the PR on the implementation outlined above (which 4 persons have thumbed up) and not some old assumption that didn't exactly work without fault on the first implementation of this. Ambiguity is a prime source for new bugs/confusion.

Admittedly I never said that it couldn't be done the way suggested in the thread. The way suggested in the thread does not take into account removing classes on the initial load of Alpine, but it can be done.

I won't be updating the PR tonight, but feel free to open a new one with a different implementation. I'll re-evaluate in the morning if following the implementation mentioned above is vital or considered a "need".

[SimoTod]

The implementation we discussed makes sense but unfortunately we don't own the project and there's a specific test to ensure that the existing behaviour is maintained so this decision is down to Caleb since it would be a breaking changes (although I doubt it would affect many people).

[bep]

Oh, well, then it would be good if @calebporzio could join in on these discussions instead of having us guessing at his intentions. The current implementation is already broken, and a broken glass is kind of a binary thing -- I would look at this as a chance to get it right. But it's not a big thing, the main thing to get fixed is what we discussed earlier, some odd behaviour I can live with.

[muzafferdede]

To summarize our discussion and bring everyone to same page , we end up with 2 scenarios.

• A: Original classes never gets removed via Alpine. Classes first get reset to origin then added only the new classes that evaluates to true.
• B: Including original classes, classes are removed/added via Alpine that evaluates to true / false.

[calebporzio]

I think I'm for mirroring Vues functionality.

I don't recall off hand why we added the remove behavior.

Because this behavior was intentional, I don't want to change it for v2.

I'm happy with putting the vue behavior on the v3 road map.

My guess why we did it this way:

• so you can "seed" initial html states. For example:

<div class="hidden" :class="{ 'hidden' : show }" >

[ryangjchandler]

@calebporzio Can we merge in the current fix for v2 and re-evaluate for v3?

[ryangjchandler]

@calebporzio Can we merge in the current fix for v2 and re-evaluate for v3? I'd be happy to make the subsequent PR for v3.

[bep]

<div class="hidden" :class="{ 'hidden' : show }" >

So, I agree that the above construct would _maybe_ make sense in some situations, but I think this is much easier to understand:

<div :class="{ 'hidden' : !show }" >

But the initial bug report was not about the case above. These constructs are painful/hard/impossible to currently get right in more complex setups:

  <button x-bind:class="{ 
                          'red text-yellow' : open,
                          'red text-blue' : !open && level === 12,
                        }">Button</button>

You end up with lots of head-scratching and "why isn't my button red?" situations ... It's not how I read the docs on x-bind and I would really hope/appreciate if this got a fix soon (as in: not in some distant v3 version).

[HugoDF]

@bep correct me if I'm wrong but there's userland workaround by setting each class individually in that expression:

```html
Button

[bep]

@HugoDF the above example is a codepen example, not real. With the 2 cases above it's manageable, but raise that to 10-20 and you get dizzy real fast.

What I want to write is:

• if x and y then make it bold and blue
• if a and b make it bold and red
• if a and !b make it italic and red
• ...

Without having to worry about statement 12 removing the bold class I added in statement 5. I get dizzy just thinking about it.

[SimoTod]

Ryan's PR should fix the bug without changing the current behaviour.
Would it work for you @bep?
V3 would just improve it further so it will be consistent with what vue does.

[bep]

Would it work for you @bep?

Yes, his PR (as described) would work great.

[ryangjchandler]

Would it work for you @bep?

Yes, his PR (as described) would work great.

Glad to hear it. I agree that the fix should be implemented for v2 and behaviour changed in v3. This thread got very opinionated and blunt quite quickly, but I'm glad we've all come to a similar conclusion that the v2 fix _just_ needs to work, then v3 can fix behaviours and inconsistencies.

[bep]

@ryangjchandler I'm sorry about the ... bluntness, but this issue was written after a good and frustrating hour inside Chrome's dev console trying to figure out why my CSS classes didn't apply, looking mostly in the wrong direction (CSS specificity ...).

[ryangjchandler]

@ryangjchandler I'm sorry about the ... bluntness, but this issue was written after a good and frustrating hour inside Chrome's dev console trying to figure out why my CSS classes didn't apply, looking mostly in the wrong direction (CSS specificity ...).

Don't stress it, fully understand having been down that rabbit hole many a time. Thanks for being honest and voicing your opinions though, that's what really matters!