Skip to content

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.

Sign up for GitHub

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

Open
trusktr wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix bug that caused changedCallback to fire instead of disconnectedCallback #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
764c3c7
Fix issue that made empty string attribute values trigger changedCall…
trusktr Oct 25, 2017
e53f71a
Don't change this.value when triggering disconnectedCallback so that …
trusktr Oct 25, 2017
a2f9e1c
Merge branch 'master' into patch-1
trusktr Aug 5, 2019
89ed312
add a prepare script to make it consumable via git
trusktr Jul 7, 2020
416d127
add a .npmignore
trusktr Jul 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .npmignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
**/*

!src/
!pkg/
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
},
"scripts": {
"build": "pack build",
"test": "testee test/connect.html test/disconnect.html test/change.html --browsers firefox"
"test": "testee test/connect.html test/disconnect.html test/change.html --browsers firefox",
"prepare": "npm run build"
},
"repository": {
"type": "git",
Expand Down
3 changes: 1 addition & 2 deletions src/registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,7 @@ class CustomAttributeRegistry {
}
}
// Attribute was removed
else if(newVal == null && !!inst.value) {
Copy link
Author

@trusktr trusktr Aug 5, 2019

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.

inst.value = newVal;
Copy link
Author

@trusktr trusktr Aug 5, 2019

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).

else if(newVal == null) {
if(inst.disconnectedCallback) {
inst.disconnectedCallback();
}
Expand Down