Weapon ranking service abstraction#1620
Conversation
…hed stores to make the bulk call).
Theoretically, the stores have actual items in them.
…into DestinyItemManager/dev
This isn't Hungarian notation, honest.
Dumping cruft and successfully calling.
bhollis
left a comment
bhollis
left a comment
There was a problem hiding this comment.
Nice, getting better. I have a few more comments.
| /** | ||
| * Fetch the DTR community scores for all weapon items found in the supplied stores. | ||
| * | ||
| * @param {any} storesContainer |
There was a problem hiding this comment.
This doesn't really help me. It doesn't say anything about what storesContainer is or what properties it might have.
There was a problem hiding this comment.
I can tell you what it walks and quacks like (an object that contains a property named stores that has all of the stores in it) but I can't tell you what the type is.
I don't know if it's an odd byproduct of how objects get passed from promise to promise or what.
| var self = this; | ||
| var allDtrItems = _.map(allItems, function(item) { return self._itemTransformer.translateToDtrWeapon(item); }); | ||
| var allKnownDtrItems = scoreMaintainer.getItemStores(); | ||
| var allDtrItems = allItems.map(function(item) { return self._itemTransformer.translateToDtrWeapon(item); }); |
There was a problem hiding this comment.
Arrow functions allow you to avoid self.
| var unmatched = allDtrItems.filter(function(dtrItem) { | ||
| var matchingItem = _.findWhere(allKnownDtrItems, { referenceId: String(dtrItem.referenceId), roll: dtrItem.roll }); | ||
| return (matchingItem === null); | ||
| }); |
There was a problem hiding this comment.
const unmatched = _.reject(allDtrItems, (dtrItem) => _.any(allKnownDtrItems, { referenceId: String(dtrItem.referenceId), roll: dtrItem.roll }));| * | ||
| * @param {any} stores | ||
| * @param {ReviewDataCache} reviewDataCache | ||
| * @returns {array} |
There was a problem hiding this comment.
Stuff like @returns {array} doesn't offer much value as documentation.
| dtrWeapons.forEach(function(dtrWeapon) { | ||
| if (!self._isKnownWeapon(list, dtrWeapon)) { | ||
| list.push(dtrWeapon); | ||
| } |
There was a problem hiding this comment.
const list = new Set(dtrWeapons);
return Array.from(list); // or leave it as a Set, it'll iterate fine|
|
||
| dtrItem.referenceId = String(dtrItem.referenceId); | ||
|
|
||
| return _.findWhere(this._itemStores, { referenceId: dtrItem.referenceId, roll: dtrItem.roll }); |
There was a problem hiding this comment.
We should make this a map (object) lookup instead of a linear scan.
| } | ||
|
|
||
| return matchingItem; | ||
| return (this._getMatchingItem(item) || null); |
| * Do we have any locally-cached review data for the given item from the DIM store? | ||
| * | ||
| * @param {any} item | ||
| * @returns {any} |
There was a problem hiding this comment.
Useless docs. The description suggests it'll return a boolean, but it looks like it returns an item (or null). @returns and @param aren't helping at all.
There was a problem hiding this comment.
I saw the note on getMatchingItem() - it's not in translateToDtrWeapon because their API calls for reference IDs to be ints, it's just returning a string in some cases (which I've let them know about).
| var tenMinutes = 1000 * 60 * 10; | ||
| var self = this; | ||
|
|
||
| setTimeout(function() { |
There was a problem hiding this comment.
Same comments about the task for purging the cache. This should be on the retrieval side, not a setTimeout.
| debugMoves: false, | ||
| // show changelog toaster | ||
| changelogToaster: $DIM_FLAVOR === 'release' || $DIM_FLAVOR === 'beta', | ||
| sendingWeaponDataEnabled: true, |
There was a problem hiding this comment.
This should be either false or $DIM_FLAVOR === 'dev'.
|
Cool. I think there's more work to do, but this is something we can merge and iterate on in the dev branch. |
3 participants
There were some pain points in the calling to DTR - it was being chattier than it needed to be (knowing that even if you submit data, it's 10 minutes before it'll be reflected) and the way it was laid out, community scores were flickering when the item stores got repainted. I've been informed that could drive lesser folks a little batty!
Also, if you didn't submit your review data and a store refresh happened in the background, that would be lost. Even if you did submit it, it would appear lost for 5-10 minutes because of the lag. These are things that could lead to unhappy support tickets.
So I built out a persistence class. When blur events are caught, user review data's updated (so it's kept around). And community scores are attached there. And community reviews are kept there as well, so if you open and re-open the same item, we won't hammer DTR with requests for data we already have.
There's cache invalidation in it which means I've almost certainly done something wrong.