if you pass in a jquery selector that does not exist, a js error results. #1

Closed
dperrymorrow opened this Issue Jun 13, 2012 · 13 comments

Projects

None yet

3 participants

@dperrymorrow
$.scrollTo('#i-dont-exist', {duration: 300});

TypeError: Cannot read property 'slice' of undefined

@nitriques

I can confirm this bug.

@flesler flesler was assigned Jul 6, 2012
@flesler
Owner
flesler commented Jul 6, 2012

I'll get this done asap, originally I left it this way because I was afraid of failing silently on a situation like this, but considering jQuery does that a lot, I think it'll be fine.

@nitriques

Thank you Ariel. BTW, thanks for this awesome jquery plugin.

@nitriques

@flesler You could do something I borrowed from jquery cycle, i.e. use a safe console.log call to write that no element where found using the provided selector.
https://github.com/malsup/cycle/blob/master/jquery.cycle.all.js#L58

@flesler flesler added a commit that referenced this issue Aug 15, 2012
@flesler Fix for #1 and #4 747f077
@flesler
Owner
flesler commented Aug 15, 2012

Can you confirm 747f077 fixes the issue in your page?

@nitriques

Just by reading the code, it should !
I will update to this new version in my projects soon and will get back to you.

@flesler
Owner
flesler commented Aug 16, 2012

747f077 had a bug, just added a new commit to address it.

@flesler flesler added a commit that referenced this issue Aug 22, 2012
@flesler Improved fix from #1
Bumped to 1.4.3
Demos and tests now use jQuery 1.8.0
Fixed a small visual bug on the demo
Added minified version
d6cce62
@flesler
Owner
flesler commented Aug 22, 2012

This is fixed, note that passing null does break and is not a supported value.

@flesler flesler closed this Aug 22, 2012
@nitriques

@flesler Passing null to jQuery works, so it should not break the plugin neither.

$(null) returns an empty array. A simple check to the length property should to the trick...

if (!!this && !!this.length) { ... }

@flesler
Owner
flesler commented Aug 22, 2012

Do you think passing a null target is a valid "intention"? as in, a
selector that doesn't match means you can use the same code on pages that
won't matter and fine with it, but passing null? why should we allow it?

On Wed, Aug 22, 2012 at 2:07 PM, Nicolas Brassard
[email protected]:

@flesler https://github.com/flesler Passing null to jQuery works, so it
should not break the plugin neither.

$(null) returns an empty array. A simple check to the length property
should to the trick...

if (!!this && !!this.length) { ... }


Reply to this email directly or view it on GitHubhttps://github.com/flesler/jquery.scrollTo/issues/1#issuecomment-7941475.

Ariel Flesler

@nitriques

@flesler First of all, I have written a lot of jQuery plugins, I and always try to implement the same behavior as jQuery did. If jQuery have a certain behavior (even if it's weird one) I try to cope with it in order to make the use of the plugin as easy as jQuery itself. So just for that reason, I would allow $(null) since it is a perfectly valid call.

On top of this, passing a null could be a valid intention. I build a lot of complex app, based on CMSes. Let say I give the ability to the user of the CMS to specify some selectors in a field, and that this field is non mandatory. Suppose I output this value as a data-selector attribute on a DOM Element. If the attribute if not present, calling $(element.data('selector')) would break and I would have to add an extra if in order to check if my selector is valid and it breaks the implementation of the Null Object Pattern implemented in jQuery.

Less line of code means better maintainability. :) But in the end, it's your choice !

@flesler
Owner
flesler commented Aug 22, 2012

Well, the data() example is a valid one I think. I'll add the change

On Wed, Aug 22, 2012 at 4:11 PM, Nicolas Brassard
[email protected]:

@flesler https://github.com/flesler First of all, I have written a lot
of jQuery plugins, I and always try to implement the same behavior as
jQuery did. If jQuery have a certain behavior (even if it's weird one) I
try to cope with it in order to make the use of the plugin as easy as
jQuery itself. So just for that reason, I would allow $(null) since it is
a perfectly valid call.

On top of this, passing a null could be a valid intention. I build a
lot of complex app, based on CMSes. Let say I give the ability to the user
of the CMS to specify some selectors in a field, and that this field is non
mandatory. Suppose I output this value as a data-selector attribute on a
DOM Element. If the attribute if not present, calling
$(element.data('selector')) would break and I would have to add an extra
if in order to check if my selector is valid and it breaks the
implementation of the Null Object Patternhttp://en.wikipedia.org/wiki/Null_Object_patternimplemented in jQuery.

Less line of code means better maintainability. :) But in the end, it's
your choice !


Reply to this email directly or view it on GitHubhttps://github.com/flesler/jquery.scrollTo/issues/1#issuecomment-7945597.

Ariel Flesler

@nitriques

Great :) Thank you !

Making a nice API (or plugin in this case) makes you responsible for checking all possibles inputs.
Javascript being weakly-typed, I strongly encourage you to tests your code against all possible values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment