patrickkunka
Sorting mixer elements throw an error
When calling a .sort in the mixer list, I was occasionally seeing an error 'attrA.toLowerCase is not a function'. I managed to track down why I was causing this exception and resolved it on my side- I did not set the attribute value for a mixer node that was to be sorted. However in looking at the mixer.js code in the compare() function line 910, line 916 does a isNaN on the 2 attributes values. Since I was missing the desired attribute on one of the targets, your getAttributeValue() will do a safe return of 0 (type number and not a type string).
Line 916 if (isNaN(attrA * 1) || isNaN(attrB * 1)) {
will pass the if() because AttrB resolves to be NaN and you are comparing using an ||. Since attrA is 0, line 917
attrA = attrA.toLowerCase();
fails because there is no toLowerCase function for a type number. My guess is that you really meant to say on line 916 is:
if (isNaN(attrA * 1) && isNaN(attrB * 1)) {
Hi @cwalker-reveille,
Thanks for raising this and taking the time to look into the code!
Yeah - I can see that that method makes a somewhat dangerous assumption that if either of the attribute values to compare is a string, then both must be a string.
This is the expected behaviour though, if I remember correctly, the intention was that if any single value is not "number-like", then sort everything as strings rather than as numbers (since everything in the DOM is a string - duck typing is the best we can do). Basically, the function expects that all attribute values coming from the DOM must be strings which would be a sound assumption if it weren't for that default value of 0.
So, I think it might be better just to change getAttributeValue() should really return an empty string, not 0.
I'm curious - there is supposed to be a warning in the console if any target elements to be sorted have missing sort attributes. Did you see it?
Hi,
No I did not see the warning message. However looking more into the code, I think you do not throw the warning because I do have the attribute defined but I did not set a value to it.
The sort attribute was ‘data-name’ so if done correctly, data-name=”myname” works fine however when I didn’t set the value the attribute in the dom was just data-name.
In any case, setting the ‘safety’ value to an empty string instead of 0 would eliminate the exception being thrown in your code.
From: Patrick Kunka [email protected] Sent: Wednesday, October 23, 2019 3:10 PM To: patrickkunka/mixitup [email protected] Cc: cwalker-reveille [email protected]; Mention [email protected] Subject: Re: [patrickkunka/mixitup] Sorting mixer elements throw an error (#522)
Hi @cwalker-reveille https://github.com/cwalker-reveille ,
Thanks for raising this and looking into the issue.
Yeah - I can see that the code makes a somewhat dangerous assumption that if either of the attribute values to compare is a string, then both must be a string.
This is the expected behaviour though, if I remember correctly, the intention was that if any single value is not "number-like", then sort everything as strings rather than as numbers (since everything in the DOM is a string - duck typing is the best we can do). Basically, the function expects that all attribute values coming from the DOM must be strings which would be a sound assumption if it weren't for that default value of 0.
So, I think it might be better just to change getAttributeValue() should really return an empty string, not 0.
I'm curious - there is supposed to be a warning in the console if any target elements to be sorted have missing sort attributes. Did you see it?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/patrickkunka/mixitup/issues/522?email_source=notifications&email_token=ALDATF6C2XV7OUHYGWJR453QQCOOXA5CNFSM4JCIJD32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECCRI3A#issuecomment-545592428 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ALDATF6HF45CTRFDGGMYYQTQQCOOXANCNFSM4JCIJD3Q . https://github.com/notifications/beacon/ALDATF5LNDCY4RHX5ALIKF3QQCOOXA5CNFSM4JCIJD32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECCRI3A.gif
This email has been checked for viruses by AVG. https://www.avg.com
Ah thanks for the info - that makes sense and seems like something else to improve too.
I'll leave this open as a bug and hope to roll out a fix sometime soon.
Thanks!
From: Patrick Kunka [email protected] Sent: Wednesday, October 23, 2019 4:22 PM To: patrickkunka/mixitup [email protected] Cc: cwalker-reveille [email protected]; Mention [email protected] Subject: Re: [patrickkunka/mixitup] Sorting mixer elements throw an error (#522)
Ah thanks for the info - that makes sense and seems like something else to improve to.
I'll leave this open as a bug and hope to roll out a fix sometime soon.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/patrickkunka/mixitup/issues/522?email_source=notifications&email_token=ALDATF4BZTHK5BV26SI6PN3QQCW5FA5CNFSM4JCIJD32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECCX2RA#issuecomment-545619268 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ALDATF5S4QGRBQGUCLEU4QDQQCW5FANCNFSM4JCIJD3Q . https://github.com/notifications/beacon/ALDATFY4VOCCMEPJGK4RWN3QQCW5FA5CNFSM4JCIJD32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECCX2RA.gif
This email has been checked for viruses by AVG. https://www.avg.com