search

xavi- / node-simple-rate-limiter

A simple way to rate limit how often a function is executed.
76 stars 10 forks source link

Function arity changes #3

Closed robludwig closed 1 year ago

robludwig commented 8 years ago

Fixes #2.

I needed this for a fix of my own. It adds some helpful behavior to the object returned by the rate limiter. It's probably an edge case and not of interest to many but I found it useful.

Tests all pass. I studied the methodology of blakeembrey/arity in crafting the solution.

xavi- commented 8 years ago

Hello, thanks for the PR.

Personal bias, but metaprogramming kinda creeps me out. That said, I certainly see the value of making sure the function arity is consistent. Perhaps we could use this technique instead?

Object.defineProperty(limiter, "length", { value: fn.length })

Wrote a more detailed explanation here: http://stackoverflow.com/questions/7316688/how-to-programmatically-set-the-length-of-a-function/37645357#37645357

It'd be less code and wouldn't require metaprogramming. The down side is that this trick doesn't work in all versions of node.js which is a bummer. We'd have to wrap the call in a try/catch to make sure we don't break people's existing code. What do you think?

robludwig commented 8 years ago

I share your wariness of metaprogramming, and defineProperty works perfectly for my purposes. I updated the PR to use that instead and did some testing to see which versions it works for. It looks like node with version >= 3 should be fine. Otherwise it fails with a TypeError. I decided to just catch and discard the error since it doesn't seem hugely important if it fails.

robludwig commented 8 years ago

Hi- I fixed this to use defineProperty and to check for errors when doing so. Anything else needed before merging?

markstos commented 3 years ago

LGTM.

xavi- commented 1 year ago

Sorry I dropped this on the flow. Thank you!