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

:class fails when the evaluated value is undefined #825

Closed
bbugh opened this issue Oct 14, 2020 · 8 comments · Fixed by #847
Closed

:class fails when the evaluated value is undefined #825

bbugh opened this issue Oct 14, 2020 · 8 comments · Fixed by #847

Comments

@bbugh
Copy link
Contributor

bbugh commented Oct 14, 2020

Working on some validation code, sometimes the result of the errorClass() doesn't require a value, e.g. is-valid and is-invalid should only be shown when the field is dirty. It's possible to return an empty string, but it would be nice if alpine handled the undefined value rather than raising a split error.

It appears that the issue is here, which does not check if the value is present:

alpine/src/utils.js

Lines 180 to 182 in 999356a

export function convertClassStringToArray(classList, filterFn = Boolean) {
return classList.split(' ').filter(filterFn)
}

When value is not an object or array, I believe this code is called in handleAttributeBindingDirective:

const originalClasses = el.__x_original_classes || []
const newClasses = convertClassStringToArray(value)
el.setAttribute('class', arrayUnique(originalClasses.concat(newClasses)).join(' '))

I'm not sure what the repercussions of changing this are. It seems like the right thing to do is to fix

I would submit a PR but this section of the code seems to be untested, and I'm not sure of the implications to change it. It seems like the right thing to do is change convertClassStringToArray to handle the non-splitable value and return an empty array or undefined, not sure which.

@HugoDF
Copy link
Contributor

HugoDF commented Oct 15, 2020

@bbugh class binding is tested in the bind tests.

We could default to an empty string in the case of class alternatively we could point this out in the README.

Up to @calebporzio on what he prefers.

@calebporzio
Copy link
Collaborator

Yeah, I think defaulting to an empty string if undefined is the right move to make the .split work and not throw a fit.

@calebporzio
Copy link
Collaborator

Closing this - I'd welcome a PR

@bbugh
Copy link
Contributor Author

bbugh commented Oct 24, 2020

I am attempting to PR this since it seems to be an easy fix, but for the life of me I cannot write a test that fails with the split error. Jest is consuming all console.log and throws in every configuration I've tried so I'm not seeing any way to test it accurately. Even adding a throw to start() doesn't fail.

test.only('undefined class binding is ignored', async () => {
    document.body.innerHTML = `
        <div x-data="{ errorClass: (hasError) => { if (hasError) { return 'red' } } }">
            <span id="error" x-bind:class="errorClass(true)">should be red</span>
            <span id="empty" x-bind:class="errorClass(false)">should be empty</span>
        </div>
    `
    // // Without the fix, this should throw error about slice here, but doesn't
    Alpine.start()

    // // Also does not throw
    // expect(() => { Alpine.start() }).toThrow()
        
    // Also does not throw
    // expect(Alpine.start).toThrow()

    // Also does not throw
    // expect(() => {
    //     Alpine.start()
    
    //     expect(document.querySelector('#error').classList.value).toEqual('red')
    //     expect(document.querySelector('#empty').classList.value).toEqual('')
    // }).toThrow()

    // This test technically fails, but not in the expected way with an Error
    expect(document.querySelector('#error').classList.value).toEqual('red')
    expect(document.querySelector('#empty').classList.value).toEqual('')
})

I confirmed that this reproduces the error in CodeSandbox (shows up in the main window browser console, not the codesandbox console): https://codesandbox.io/s/bold-bird-qklky?file=/index.html:185-463

Is the test configuration set up some how not to catch errors? It seems like there could be other secretly failing tests if this is the case.

@SimoTod
Copy link
Collaborator

SimoTod commented Oct 24, 2020

start() is an async function so you can't check if an exception is thrown in that way (see jestjs/jest#1377 and https://jestjs.io/docs/en/asynchronous).

What you need is something like

test('undefined class binding resolves to empty string', async () => {
    document.body.innerHTML = `
        <div x-data="{ errorClass: (hasError) => { if (hasError) { return 'red' } } }">
            <span id="error" x-bind:class="errorClass(true)">should be red</span>
            <span id="empty" x-bind:class="errorClass(false)">should be empty</span>
        </div>
    `

    // Test that Alpine.start resolves correctly (If an error is thrown, the promise will be rejected)
    await expect(Alpine.start()).resolves.toBeUndefined() //<-- because start doesn't return anything

    expect(document.querySelector('#error').classList.value).toEqual('red')
    expect(document.querySelector('#empty').classList.value).toEqual('')
})

@bbugh
Copy link
Contributor Author

bbugh commented Oct 25, 2020

@SimoTod thanks! Unfortunately, that also does not result in an error, but it should, based on the reproduction in the code sandbox.

 PASS  test/bind.spec.js
  ✓ undefined class binding resolves to empty string (24ms)

Pasted_Image_10_25_20__7_32_AM

commit 56443ea715afcb0139b46f9de14a9432a952300e (HEAD -> master, tag: v2.7.2, origin/master, origin/HEAD)
Author: Caleb Porzio <[email protected]>
Date:   Thu Oct 22 10:50:24 2020 -0400

    Publish 2.7.2

@SimoTod
Copy link
Collaborator

SimoTod commented Oct 25, 2020

Yeah, I wrote "something like" because it wasn't the final implementation since I was typing from mobile, and I didn't test it. 😄

You also need to take into account that errors are deferred in Alpine (see https://github.com/alpinejs/alpine/blob/master/src/index.js#L95).
The easiest way for me would be to make setTimeout sync in your test ->
master...SimoTod:misc/x-bind-null-class and https://github.com/SimoTod/alpine/runs/1305125541?check_suite_focus=true

@bbugh
Copy link
Contributor Author

bbugh commented Oct 25, 2020

Ah ha! I did not see the setTimeout deferment when I was digging through the code. It makes sense why I couldn't find the error. At first I thought I was going crazy, like did I forget how to write tests?! 🤪

I made #847 for this.

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 a pull request may close this issue.

4 participants