Sorry, did not post the complete fix. It somehow seems necessary to convert the string to an actual number on that line. Not sure why this is actually needed, but should (IMO) not be problematic as it now does what the user wants.

!isNaN(opt.duration) ? parseFloat(opt.duration)

Since this could be solved by sanitizing the input from the text, I don't think we need extra code to convert a string.

I agree that the input could be sanitized, but I still think jQuery does the wrong thing in this situation. IMO this behaviour can lead to hard to find bugs which should be avoided if feasable.

Please reconsider this bug report. The actual bug is not the conversion. It's the check (opt.duration === "number") that does the wrong thing. I'm not sure why the value then has to be an actual number (maybe that should be investigated further), but it's easy to fix it here.

To support the case I give you a few arguments why I think this should be changed.

1. I think it makes sense for the animation API to accept numbers also from strings if valid. Only lookup the speed otherwise and use the default if nothing matches. Setting an option to '5000' or 5000 and getting different results just doesn't feel right in a dynamic typed language.

2. Currently if you set jQuery.fx.speeds{'5000'} = 1500, a speed of '5000' will result in a duration of 1500ms. A speed of 5000 will result in a duration of 5000ms. And a speed of '500' will use the default duration. I'm not sure if this possibility is actually intended, but I doubt it.

3. The code changes should not have a big performance impact.

4. It would protect the coder from a possible pitfall.

Of course that's just my opinion. But I don't see a reason why this could or should not be fixed (enhanced).

Best wishes

Marcel Greter

I think it makes sense for the animation API to accept numbers also from strings if valid.

Does it make sense for every single API that accepts numbers to accept strings? If not, why does .animate() deserve special treatment?

Setting an option to '5000' or 5000 and getting different results just doesn't feel right in a dynamic typed language.

Dynamic typing doesn't mean you can be willy-nilly with your types. This is like using 1 instead of true just because it can work. That doesn't mean that all places that accept a boolean should accept truthy/falsey values.

Currently if you set jQuery.fx.speeds{'5000'} = 1500, a speed of '5000' will result in a duration of 1500ms. A speed of 5000 will result in a duration of 5000ms. And a speed of '500' will use the default duration. I'm not sure if this possibility is actually intended, but I doubt it.

This is purely a hypothetical. Nobody would ever do such a thing, and if they did, then they clearly meant it.

The code changes should not have a big performance impact.

How about bytes? Don't forget to multiply for every single place a number is accepted since this would set a precedent.

It would protect the coder from a possible pitfall.

We can't protect from everything. The coder needs to take some responsibility. Strings are a valid form of input, so we're not going to special case certain string values.

Replying to [comment:4 scott.gonzalez]:

> I think it makes sense for the animation API to accept numbers also from strings if valid. Does it make sense for every single API that accepts numbers to accept strings? If not, why does .animate() deserve special treatment?

IMO all APIs do accept numbers from strings when they expect a number. And not only numbers of the actual type number, but also if they are of type string. Like window.setTimeout(fn, '1000') or jQuery('#id').offset({ top: '400' }). The Problem is that Animate has a check (typeof opt.duration === "number") that leads to the IMO unexpected behaviour.

  opt.duration = jQuery.fx.off ? 0 : typeof opt.duration === "number" ? opt.duration :
    opt.duration in jQuery.fx.speeds ? jQuery.fx.speeds[ opt.duration ] : jQuery.fx.speeds._default;

Sorry I wasted everybody's time!

Have a nice day

Marcel Greter

Hi there

I thought about resting this case, as I now know this pitfall. But for the better of jQuery, I insist that this report is taken seriously! Altough it might be of marginal impact.

Replying to [comment:4 scott.gonzalez]:

> Setting an option to '5000' or 5000 and getting different results just doesn't feel right in a dynamic typed language. Dynamic typing doesn't mean you can be willy-nilly with your types. This is like using 1 instead of true just because it can work. That doesn't mean that all places that accept a boolean should accept truthy/falsey values.

If you use a check like "if (isEnabled)" it doesn't matter if you have 1, '1', 'asdqwe' or true, the check will always be true as long as dynamic typing resolves the value to a boolean that is true. But it doesn't if you write "if (isEnabled === true)". Why would you want to implement strict type checking everywhere? Why is it that we only have ~ 5 such typeof checks in the whole jQuery library? For me typeof is a feature that may should not even be in the specs of ECMA, but if you use it, you should know what you do! IMO it's confusing to the API user if you act differently for '5000' and 5000, merely because of the current type of the variable. That's something for C, C++ or maybe Java, but IMO not JavaScript. And comparing 1 to true is IMO not really the same as comparing '5000' to 5000. Compare '1' to 1 and explain me why one animation should run for 400ms and the other only for 1ms.

> Currently if you set jQuery.fx.speeds{'5000'} = 1500, a speed of '5000' will result in a duration of 1500ms. A speed of 5000 will result in a duration of 5000ms. And a speed of '500' will use the default duration. I'm not sure if this possibility is actually intended, but I doubt it. This is purely a hypothetical. Nobody would ever do such a thing, and if they did, then they clearly meant it.

Of course nobody would do such a thing! Why should one!? But it may be the only valid point to keep it the way it is now, as with the fix this behaviour would no longer exist. That was the only reason why I mentioned it!

> The code changes should not have a big performance impact. How about bytes? Don't forget to multiply for every single place a number is accepted since this would set a precedent.

As I've written more than once, it's not the conversion but the check that is the bug. So it doesn't mean this needs to be implemented everywhere, it's really just a bug about the speed/animation duration. And if you're talking about bytes, the check for any typeof call doesn't need a tripple equal sign to compare (a double equal sign should be enough as typeof will always return a string, as far as I know). If one would also optimize the ternary operator, I think it will use maybe +-2 bytes more than before!

  opt.duration = jQuery.fx.off ? 0 : typeof opt.duration === "number" ? opt.duration :
    opt.duration in jQuery.fx.speeds ? jQuery.fx.speeds[ opt.duration ] : jQuery.fx.speeds._default;

  opt.duration = jQuery.fx.off ? 0 : !isNaN(opt.duration) ? parseFloat(opt.duration) :
    opt.duration in jQuery.fx.speeds ? jQuery.fx.speeds[ opt.duration ] : jQuery.fx.speeds._default;

> It would protect the coder from a possible pitfall. We can't protect from everything. The coder needs to take some responsibility. Strings are a valid form of input, so we're not going to special case certain string values.

It's true that you can't protect them from everything. But if you can protect him from one case withouth any further penalty, why not do it? I don't really get the core of this argument!

Sorry if I'm a little bit furious. But for me this clearly is a bug and should be fixed. I now know others may think different, but I'm ready to defend that case. So far no valid reasons have beeen given to not do so (IMHO)!

So to repeat the main case, I think this line:

  opt.duration = jQuery.fx.off ? 0 : typeof opt.duration === "number" ? opt.duration :
    opt.duration in jQuery.fx.speeds ? jQuery.fx.speeds[ opt.duration ] : jQuery.fx.speeds._default;

Should be changed to this line:

  opt.duration = jQuery.fx.off ? 0 : !isNaN(opt.duration) ? parseFloat(opt.duration) :
    opt.duration in jQuery.fx.speeds ? jQuery.fx.speeds[ opt.duration ] : jQuery.fx.speeds._default;

If this code change does not land, it should at least be documented that an animation duration set to '1234' will result in the default fx.speed (400). So you would have to check for "is a number" yourself and convert it to an actual number type variable!

I have no idea how I could make it any clearer; to say it bluntely, "I don't get it why you don't get it". For me this case couldn't be any clearer. But I'm still open for constructive criticism, if it's directly about this problem.

Kind regards

Marcel Greter