search

jhurliman / node-rate-limiter

A generic rate limiter for node.js. Useful for API clients, web crawling, or other tasks that need to be throttled
MIT License
1.51k stars 135 forks source link

Uses process.hrtime when possible, rather than the Date object, so that time is monotonic. #44

Closed C-Saunders closed 6 years ago

C-Saunders commented 6 years ago

This is a version of https://github.com/jhurliman/node-rate-limiter/pull/30 that falls back to using the Date object if process is not available.

Closes #22.

C-Saunders commented 6 years ago

Yes, they aren't expected to be the same. From the Node docs (emphasis added):

These times are relative to an arbitrary time in the past, and not related to the time of day and therefore not subject to clock drift. The primary use is for measuring performance between intervals

Whereas the JavaScript Date object is always relative to Jan. 1, 1970. See the MDN docs.

From what I could tell, the absolute time wasn't relevant in this project, just the interval, in which case process.hrtime may be a a better default.

jhurliman commented 6 years ago

That makes sense. Using relative time would make future serialization/deserialization slightly more challenging, but not impossible and it's not breaking a current feature today so I'll go ahead and merge. Thanks for the PR!

C-Saunders commented 6 years ago

Apparently travis changed its mind about the destructing assignment added here since it passed, but now causes build failures 👎

jhurliman commented 6 years ago

Ah yeah :-/, that should probably be changed for better browser support (without transpiling).

C-Saunders commented 6 years ago

I'll open a new PR for it shortly.