-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
π‘ [major] Prefer MSGPack over JSON #93
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,7 @@ | |
}, | ||
"dependencies": { | ||
"ioredis": "5.x", | ||
"msgpackr": "1.x", | ||
"lru-cache": "10.x", | ||
"redlock": "4.x" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
import Redis from 'ioredis'; | ||
import * as Redlock from 'redlock'; | ||
import { Packr } from 'msgpackr'; | ||
|
||
import { CachableValue, CacheInstance } from './CacheInstance'; | ||
|
||
const MPACK = new Packr({ moreTypes: true }); // `moreTypes: true` to get free support for Set & Map | ||
|
||
/** | ||
* Wrapper class for using Redis as a cache. | ||
|
@@ -21,6 +23,7 @@ export class RedisCache extends CacheInstance { | |
public static TRUE_VALUE = 'f405eed4-507c-4aa5-a6d2-c1813d584b8f-TRUE'; | ||
public static FALSE_VALUE = 'f405eed4-507c-4aa5-a6d2-c1813d584b8f-FALSE'; | ||
public static JSON_PREFIX = 'f405eed4-507c-4aa5-a6d2-c1813d584b8f-JSON'; | ||
public static MSGP_PREFIX = 'f405eed4-507c-4aa5-a6d2-c1813d584b8f-MSGP'; | ||
public static ERROR_PREFIX = 'f405eed4-507c-4aa5-a6d2-c1813d584b8f-ERROR'; | ||
public static NUMBER_PREFIX = 'f405eed4-507c-4aa5-a6d2-c1813d584b8f-NUMBER'; | ||
|
||
|
@@ -166,19 +169,10 @@ export class RedisCache extends CacheInstance { | |
} | ||
|
||
if (value instanceof Object) { | ||
return RedisCache.JSON_PREFIX + JSON.stringify(value, (key, value) => { | ||
if (value instanceof Set) { | ||
return { __dataType: 'Set', value: Array.from(value) }; | ||
} else if (value instanceof Map) { | ||
return { __dataType: 'Map', value: Array.from(value) }; | ||
} else { | ||
return value; | ||
} | ||
}); | ||
return `${RedisCache.MSGP_PREFIX}${MPACK.pack(value).toString('binary')}`; | ||
} | ||
|
||
return value; | ||
|
||
} | ||
|
||
/** | ||
|
@@ -212,6 +206,10 @@ export class RedisCache extends CacheInstance { | |
return false; | ||
} | ||
|
||
if (value.startsWith(RedisCache.MSGP_PREFIX)) { | ||
return MPACK.unpack(Buffer.from(value.substring(RedisCache.MSGP_PREFIX.length), 'binary')); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π Can you also fix the type of the function param on line 190? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it isn't related to this changeset per se, I did it in another PR. |
||
|
||
if (value.startsWith(RedisCache.ERROR_PREFIX)) { | ||
const deserializedError = JSON.parse(value.substring(RedisCache.ERROR_PREFIX.length)); | ||
// return error, restoring potential Error metadata set as object properties | ||
|
@@ -240,7 +238,6 @@ export class RedisCache extends CacheInstance { | |
} | ||
|
||
return value; | ||
|
||
} | ||
|
||
/** | ||
|
christianblais marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christianblais looks sane and no-dependencies. However, usual introducing-new-depencency questions: how did you pick it? Were there alternatives, and if yes, what led you to think it's the best one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MessagePack is a well defined standard, so the question was β "which implementation should I pick?". There aren't a ton of options to pick from, and this one stands out as the clear winner in terms of love (number/frequency of commits), popularity, and speed.
Now, I'm aware a lib might be feature-complete with no bugs, hence the lack of activity. But looking at the downloads per week, once more,
msgpackr
stood out as the clear winner. That was the end of my investigation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yip that's what I meant. Okay with your annotated screenshot!