-
Notifications
You must be signed in to change notification settings - Fork 7
Fix bug that caused changedCallback to fire instead of disconnectedCallback #17
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
base: master
Are you sure you want to change the base?
Conversation
|
Anything I should add to tests? |
|
Oops, realized I changed the wrong file. Let me edit... |
…back instead of disconnectedCallback removing an attribute
…disconnectedCallback logic can rely on the value (i.e. the attribute value didn't change, the attribute was removed)
|
Alright, updated. |
|
Thanks! I'll probably add a test to this before merging. |
|
Woops, closed by mistake. |
* master: bump travis node version build to test use @pika/pack to build
| } | ||
| // Attribute was removed | ||
| else if(newVal == null && !!inst.value) { | ||
| inst.value = newVal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning here is that this.value will show the last value that the Custom Attribute had while being disconnected.
So it's more of like null doesn't exist in the eyes of a Custom Attribute, instead it should just get disconnected. The null is a value in the eyes of an element (f.e. Custom Element), so that it knows an attribute is not connected (the custom attributes that this is the case when disconnectedCallback is called).
In my use cases, I found it useful to know what the last value was during disconnected (and I can automatically infer the DOM's is null due to disconnectedCallback, though in practice I never need the null value, because I already know when my custom attribute is disconnected and can react to that).
| } | ||
| } | ||
| // Attribute was removed | ||
| else if(newVal == null && !!inst.value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example: if a custom attribute is some-attribute="", and then it gets removed, the newVal will be null, and the current inst.value will "" which is falsy, which then causes the conditional to incorrectly skip calling disconnectedCallback.
|
I'll add a test soon... |
|
I need to circle back to this! Putting it on my Todo list this time. |
This also prevents from modifying the custom attribute's
this.valuewhen disconnected, so that the old value can be used in the disconnected logic (because the value wasn't changed, the attribute was removed).