Skip to content
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

separate out vendor component calls #10430

Merged
merged 2 commits into from
May 22, 2024
Merged

Conversation

nev-r
Copy link
Member

@nev-r nev-r commented May 18, 2024

addresses #10426 (Bungie-net/api#1887)

early sanity-checking welcome please. is this a good way to combine these?
it results in a pleasant filling-in of info, and polaroids gradually appearing on the vendors page,
but as noted in loadAllVendors comments, there are other possible strategies

this serially fetches, but that ends up taking like 50 seconds for 9 vendors.
should we ratelimit, and dispatch them all, and await allResolved? what rate?

const defs = manifestSelector(getState());
if (
// we're done if this is d1. or if defs aren't loaded?
// can this kick off before defs are loaded?? what if it does???
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be able to put this in a store observable, so that it will kick off after defs are loaded?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it always kicks off after defs are loaded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great to be reassured of, definitely appeared to that way in all testing

Copy link
Contributor

@bhollis bhollis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Comment on lines 97 to 99
// a nice long instanceId is a real one and "should" have this data
// (short is actually a vendor item index. we'll let that build from defs.)
item.itemInstanceId.length > 14 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heuristic makes me uncomfortable, but I'm not sure of a better one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the conversion for vendor items, from just a number(as a string) to ${vendorHash}-${saleIndex} happens later in item build unfortunately, or it would be more identifiable

item ids start at 69 sextillion, 19 digits, even in d1, and count up, so this is not likely to fail any time soon

Comment on lines 63 to 64
const newItemComponents = vendorResponse.itemComponents; // big combined set, keyed by vendor hash
const existingItemComponents = oldVendorsResponse?.itemComponents; // set for a single vendor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments seem backwards?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah! unsure when that swapped. combination went through several iterations. thank you

Comment on lines 67 to 76
return {
...state,
vendorsByCharacter: {
...vendorsByCharacter,
[characterId]: {
vendorsResponse: { ...oldVendorsResponse, itemComponents },
lastLoaded: Date.now(),
error: undefined,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all perfectly reasonable, though the immer version might read better:

Suggested change
return {
...state,
vendorsByCharacter: {
...vendorsByCharacter,
[characterId]: {
vendorsResponse: { ...oldVendorsResponse, itemComponents },
lastLoaded: Date.now(),
error: undefined,
},
},
return produce(state, (draft) => {
draft.vendorsByCharacter[characterId].vendorsResponse.itemComponents[vendorHash] = vendorResponse.itemComponents;
draft.vendorsByCharacter[characterId].lastLoaded = Date.now();
}),

const defs = manifestSelector(getState());
if (
// we're done if this is d1. or if defs aren't loaded?
// can this kick off before defs are loaded?? what if it does???
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it always kicks off after defs are loaded.

Comment on lines +106 to +107
// deprioritize vendors whose sections are collapsed on the vendors page
compareBy((h) => Boolean(collapsedSections[`d2vendor-${h}`])),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever!

Comment on lines 118 to 124
for (const vendorHash of vendorsNeedingComponents) {
start = Date.now();
const vendorResponse = await getVendorSaleComponents(account, characterId, vendorHash);
timings.componentsTime += Date.now() - start;
timings.componentsFetched++;
dispatch(loadedSingle({ vendorResponse, characterId, vendorHash }));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be parallelized in batches, but this is a perfectly fine start.

return (
item &&
// probably just need this for weapon perks and armor stats?
// TO-DO: what about bounties and quests? where's their pre-existing progress stored? do we care?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might affect whether bounties appear purchase-able?

Copy link
Contributor

@delphiactual delphiactual May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bounties appear correct in my testing
image

@nev-r nev-r merged commit 42648cb into master May 22, 2024
7 checks passed
@nev-r nev-r deleted the separate-vendor-component-calls branch May 22, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants