craftyjs
Allow unsanitized input (e.g. dimensions as strings)
See the following: https://jsfiddle.net/4b1L85bk/
Dragging is unusably slow because the dimensions are passed in as strings. Preferably, Crafty should sanitize the arguments by converting to numbers to prevent this.
Why would you pass in width and height as strings? What's the use case?
Further, Draggable doesn't reference height or width at all. The issue is probably elsewhere.
It happened by accident, that's the point. Arguments were obtained from the styling properties of some other DOM elements, which are strings.
With no error raised and no rhyme or reason why the simple example code works fine, and the DOM-sourced values didn't, I wasted an hour just to find out that Crafty doesn't coerce arguments to numbers.
So attr is very generalized with its arguments, so sanitizing probably shouldn't happen there.
Maybe here? If every 2d property should be a number, do this...
_setter2d: function (name, value) {
// Return if there is no change
if (this[name] === value) {
return;
}
value = Number(value)
...
My only concern is I'm not sure how often that gets called. At some point if we're enforcing types, it's worth considering javascript offshoots which do that inherently, too. Throwing an error is probably a bad idea for speed, as that would still involve a check on every set. Putting in documentation that clarifies it's the user's job to sanitize would be better for speed, too, if it's on the table.
(I made a jsPerf for the impact of number casting here).
Considering values like x and y change a lot, and considering the huge impact casting has on speed, I'd either advocate just adding some documentation clarifying that it's the user's responsibility, or I'd special-case attr (which would still let something like "x = '4'" go through as a string and isn't a good solution).
Yep, as Sythe2o0 mentioned, sanitizing inputs affects performance negatively.
However, attr isn't used that often (Crafty's internals and a performance-concerned user will set properties directly), so adding input sanitation to it (or to a sub-function of it) doesn't seem like a bad idea.
I believe that prepending a + (e.g. +'3') will convert it to a number.
Shouldn't be too difficult, If anyone wants to open a PR, we could merge that in.
I suppose Javascript doesn't leave you much choice here. I haven't found a way to efficiently check that a value is a number, so you can't even detect and throw an error in this case to inform the user. Bummer.
A performance/troubleshooting page describing these symptoms might be all you can do. That would be preferable to documenting it just on the functions, because if I'm experiencing performance problems I would have no idea where to even look.
At some point if we're enforcing types, it's worth considering javascript offshoots which do that inherently, too.
It might be interesting to see how hard it is to provide TypeScript annotations for Crafty.
It might be interesting to see how hard it is to provide TypeScript annotations for Crafty.
I wouldn't be opposed to incrementally switching over to TS (make a change, convert to TS). It naturally fosters optimizations in browsers and opens up possible future optimizations.