[bug] empty string values incorrectly cause `changedCallback` to fire instead of `disconnectedCallback` when removing an attribute

[trusktr]

I noticed that when we remove an attribute, changedAttribute gets called with null values for newVal. This was a bit confusing, as the null values were causing errors, and I didn't know where they were coming from; it wasn't something I expected.

I expected changedCallback to fire only for when values have changed, and arguments to always be strings in this regard.

We already have disconnectedCallback(), so we necessarily need changedCallback('', null) because it means we have to add defensive code to prevent logic from running when we see null values when we could otherwise just rely on disconnectedCallback and have cleaner code without having to check for null.

Is there some cases I may have missed?

Also the README doesn't mention anything about the null value, which would be good.

[trusktr]

Oops, looks like this was all my fault, I was doing this in my attribute class:

    connectedCallback() {
        this.changedCallback( '', this.value )
    }

    disconnectedCallback() {
        // this.value is null
        this.changedCallback( this.value, '' )
    }

so this.value was null and I was passing it in. I'm basically trying to run the same logic on connected/disconnected.

I see that connecting an attribute causes changedCallback to fire, but disconnecting does not, but I wanted it to.

I think a fix would be for this.value to be the current value while disconnectedCallback is being called, which makes sense: the value hasn't changed, it's only being disconnected, then I can emulate a change easily:

    disconnectedCallback() {
        // this.value is the value the attribute had, it didn't change, it was only disconnected.
        this.changedCallback( this.value, '' )
    }

[matthewp]

@trusktr Are you saying that this.value is not the current value when attribute is disconnected? What is, then?

[trusktr]

The value of this.value inside of disconnectedCallback is null.

f.e. If I have

    disconnectedCallback() {
        console.log(' ------- disconnectedCallback', this.value)
    }

then in console:

> document.querySelector('#second').removeAttribute('has')
 ------- disconnectedCallback null

[matthewp]

Ok. I wonder if there's a way to make it be null without removing the attribute / removing the dom node. If not then I would say that we can simply guard against null values to prevent this bug.

[trusktr]

Hmmmm, something funky is happening.

Sometimes when I remove a custom attribute, it calls changedCallback instead of disconnectedCallback.

For example, here's what I see in the console when adding and removing an attribute:

> document.querySelector('#second').setAttribute('has', `sphere-geometry basic-material`)
 ------ changedCallback
> document.querySelector('#second').removeAttribute('has')
 ------- disconnectedCallback null

(so far so good)

> document.querySelector('#second').setAttribute('has', `sphere-geometry basic-material`)
 ------- connectedCallback sphere-geometry basic-material
> document.querySelector('#second').setAttribute('has', ``)
 ------ changedCallback

(still good)

> document.querySelector('#second').removeAttribute('has')
 ------ changedCallback

Uh oh, I was expecting disconnectedCallback. I'm trying to find what is going on, I'm not sure if it's my fault or not yet.

[matthewp]

Thanks for investigating @trusktr. If that sequence produces a bug it should be easy to replicate in a test.

[trusktr]

On this line, https://github.com/matthewp/custom-attributes/blob/master/attr.js#L123, I see how if newVal is null then the attribute was removed, but what's the !!inst.value check for?

[trusktr]

The value of an attribute can be set to an empty string, so !!inst.value will be false which will trigger changedCallback instead of disconnectedCallback.

[trusktr]

PR: #17

[trusktr]

After that change, all my stuff seems to be working great now. I was initially setting the attribute value to an empty string if the attribute didn't exist on a custom element after construction, which caused strange behavior, and all seems to work as expected now.