feat: leaderboard bug bash fixes, updated response classes #966
Conversation
anitarua
requested review from
cprice404,
pratik151192 and
virratanasangpunth
| return convertedElements; | ||
| } | ||
|
|
||
| public async leaderboardUpsert( | ||
| public async upsert( |
There was a problem hiding this comment.
this class is internal, so it's fine to change the name here, but not crucial.
I don't see the public classes in this PR?
| @@ -38,10 +38,10 @@ export abstract class AbstractLeaderboard implements ILeaderboard { | |||
| * {@link LeaderboardUpsert.Success} on success. | |||
| * {@link LeaderboardUpsert.Error} on failure. | |||
| */ | |||
| public async leaderboardUpsert( | |||
| elements: Map<number, number> | |||
| public async upsert( | |||
There was a problem hiding this comment.
Public classes are in AbstractLeaderboard.ts
These are all inherited by the Leaderboard class in each package.
There was a problem hiding this comment.
ooohhhh right and the PreviewClient classes just have .leaderboard(). Okay you're right, my bad.
| public async leaderboardFetchByRank( | ||
| public async fetchByRank( | ||
| startRank: number, | ||
| endRank: number, |
There was a problem hiding this comment.
I hadn't thought about how making these required would cause us to pull them up to positional arguments, or else make the options argument required.
What you did here makes sense, but the one downside is that because JS doesn't allow callers to use keyword-style call patterns for positional arguments, you can no longer say startRank: blah in your call to these, it'll just be fetchByRank(12, 42), which isn't quite as readable.
but it is more succinct. and it seems fairly intuitive. so i think i'm on board with doing it this way, vs. keeping these inside the options interface and just making the options arg required.
@bruuuuuuuce I am curious if you have any opinion on this.
There was a problem hiding this comment.
:skeletor_fist_shake: nodejs 😠 .
Generally speaking, I like to use objects as params to my functions in js for this exact reason. However,
- this is more concise
- this is the pattern we currently have set for our other methods
- most js sdks just use positional args like this like the redis sdk
I think its ok to ship with this
There was a problem hiding this comment.
i'm on board with this. i'd be fine with you merging it. i'm interested in Matt's opinion about whether startRank/endRank live in the options arg or not but i would be okay with fast-follow on that if we decided to change it, and I think i'm in favor of the way you have it now.
Ticket #468. Addresses feedback we got from the bug bash.
Changes include:
FoundandNotFoundresponse classes which were confusing.Erroris returned when the cache is not found and thus the request failed.Successis returned when the cache was found, but does not necessarily mean that requested elements were found (e.g. fetching data from empty or non-existent leaderboards will return length=0 or elements=[] rather thanNotFoundnow).leaderboardprefixes from the Leaderboard methods.leaderboardUpsert-->upsertRecord<number, number> | Map<number, number>instead of justMap<number, number>.fetchByRanknow requiresstartRankandendRankfor pagination through leaderboards.Will fast follow to update the nodejs example and doc example snippets after this is released. Those haven't made it into the dev docs yet.