fix: `nvidia-api` #314

andrewmackrodt · 2020-09-25T18:01:57Z

Fixes #267 - uses the new API to check for stock status and the new API add-to-card action which generates the cart redirect URL. I've only tested this with a 2060 Super by changing the great_britain.fe3080Id value to that of an in stock 2060 Super and adding to cart and opening browser works. The actual stock detection change has been working for me for over 36 hours, i.e. it successfully has tracked 3080 and 3090 stock in the UK store, I actually purchased a 3080 6am UK time yesterday.

AFAIK the only thing missing compared to the previous behaviour was opening a page when the app is started. I've not found a way to get the cart redirection url without first adding a product to the basket. Also not tested is if previous checkouts are overwritten if someone has a low (or unset) IN_STOCK_WAIT_TIME setting.

Test script for currency update:

import {regionInfos} from './store/model/nvidia-api';
import {get} from 'request';
import {nvidiaStockUrl} from './store/model/helpers/nvidia';
import {promisify} from 'util';

async function verifyNvidiaProductsHavePricing() {
	for (const regionInfo of Array.from(regionInfos)) {
		const [country, data] = regionInfo;
		if (!data.fe3080Id) {
			continue;
		}

		const storeUrl = nvidiaStockUrl(data.fe3080Id, data.drLocale, data.currency);
		let productData: Record<string, any> | undefined;

		try {
			const response = await promisify(get)({
				headers: {
					'user-agent': 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.157 Safari/537.36'
				},
				url: storeUrl
			});

			productData = JSON.parse(response.body);

			if (typeof productData !== 'object' || productData === null) {
				throw new Error('Not a JSON object');
			}
		} catch (e) {
			console.error(`Failed to retrieve product information for country ${country}`);
			console.error(e.message);
			continue;
		}

		const price = productData.products?.product[0]?.pricing?.listPrice;

		if (price) {
			console.info(`Found price information for country ${country} => ${price.currency} ${price.value}`);
		} else {
			console.warn(`Failed to find price information for country ${country}`);
		}
	}
}

void verifyNvidiaProductsHavePricing();

thegutuproject · 2020-09-25T21:50:47Z

src/store/model/helpers/nvidia.ts

 function getRegionInfo(): NvidiaRegionInfo {
 	const country = Array.from(regionInfos.keys()).includes(Config.store.country) ? Config.store.country : 'usa';

-	const defaultRegionInfo: NvidiaRegionInfo = {drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: null, nvidiaLocale: 'en_us'};
+	const defaultRegionInfo: NvidiaRegionInfo = {currency: 'USD', drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: null, nvidiaLocale: 'en_us'};


We can populate the fe3090Id here: 5438481600

jef

AFAIK the only thing missing compared to the previous behaviour was opening a page when the app is started

Yep! This is the right approach as of now (see Hari-Nagarajan/fairgame#128).

I really appreciate you putting in this work! This was next on my to-do list and you've got it all sorted!

jef · 2020-09-25T21:53:46Z

src/store/model/helpers/nvidia.ts

+
+	await page.setRequestInterception(true);
+
+	page.on('request', interceptedRequest => {


Nice, this will speed this up quite a bit. Was looking for this type of API in puppeteer. Nice find!

Sorry if I'm being dense, but what is the requested change?

Oh.. I didn't send :o

Lol I had two reviews open... Whoops. Thanks!

jef · 2020-09-25T22:09:35Z

src/store/model/helpers/nvidia.ts

 function getRegionInfo(): NvidiaRegionInfo {
 	const country = Array.from(regionInfos.keys()).includes(Config.store.country) ? Config.store.country : 'usa';

-	const defaultRegionInfo: NvidiaRegionInfo = {drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: null, nvidiaLocale: 'en_us'};
+	const defaultRegionInfo: NvidiaRegionInfo = {currency: 'USD', drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: null, nvidiaLocale: 'en_us'};


May have been a copy paste error, but I think we do have a 3090 code here. You've got it down below at least! 😄

Suggested change

const defaultRegionInfo: NvidiaRegionInfo = {currency: 'USD', drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: null, nvidiaLocale: 'en_us'};

const defaultRegionInfo: NvidiaRegionInfo = {currency: 'USD', drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: 5438481600, nvidiaLocale: 'en_us'};

It is null in the existing HEAD. I've modified the function to remove duplication by checking if the regions map contains the country and falling back to usa if not.

jef · 2020-09-25T22:23:07Z

src/store/model/helpers/nvidia.ts

+	if (!regionInfo) {
+		throw new Error(`LogicException could not retrieve region info for ${country}`);
+	}


I think you can remove this as you already check in lines 11-13?

Better approach though. Thanks!

I think you can remove this as you already check in lines 11-13?

I think it needs to be checked anyway or the TypeScript compiler will correctly throw an error that regionInfo may be undefined, e.g. if usa was renamed or removed.

Based on that assumption, I suppose you could do the opposite then and do one call.

if (!regionInfo) { country = 'usa'; }

And remove the previous if statement 🤷

I may not be following, but the only way this will work without 2 if conditions is if we silence the typescript compiler, e.g.

function getRegionInfo(): NvidiaRegionInfo { let regionInfo = regionInfos.get(Config.store.country); if (!regionInfo) { regionInfo = regionInfos.get('usa') } return regionInfo!; }

It has no way to tell that usa will always exist in the regionInfos map, and it's IMO correct to behave that way.

Ah, got it. No problem. I haven't pull your branch, so I guess I haven't seen what the compiler spits out yet. Makes sense to me!

Ah. I see now. I come from a different language so the idiosyncrasies are still catching up.

I was thinking usually when you call T.get() or us [], if it can't find the object, it'll throw an exception. I see that Map.get() in TS will just return an undefined object, rather than throw.

My assumption is that we would do something like this:

function getRegionInfo(): NvidiaRegionInfo { const regionInfo = regionInfos.get(Config.store.country); if (!regionInfo) { return regionInfos.get('usa') } return regionInfo; }

But I see the TypeAssertion error now.

Below isn't a request to change, just some feedback would be nice for a learning experience on my end if you didn't mind!

Would something like this run fine?

function getRegionInfo(): NvidiaRegionInfo { const regionInfo = regionInfos.get(Config.store.country); if (!regionInfo) { return <NvidiaRegionInfo>regionInfos.get('usa') } return regionInfo; }

It compiles, but as long as we know 'usa' is a provided country, we could. This could still be dangerous as 'usa' may "suddenly" disappear.

I suppose in that case we could create a wrapper function around Map.get() for getting a default NvidiaRegionInfo and avoid altogether!

That would work, TS allows type coercion so long as the type is legal. Another way to write that would be regionInfos.get('usa') as NvidiaRegionInfo, or regionInfos.get('usa')! to tell the compiler it's definitely defined. The last one would be most applicable to this example, type coercion is only needed to tell the compiler the result is a one of multiple defined types that are valid.

Got it. So best case is to use the implementation you had, but next best is the regionInfos.get('usa')!. Makes sense to me. I suppose it lacks a keyword unsafe 😉

Thanks for the explanation!

manybot · 2020-09-25T22:29:14Z

Out of curiosity, would this require then running nvidia-api as the "store" as it is stored in model, or nvidia which inquires nvidia-api which uses the helper nvidia.ts (sub folder ) ?

which should the .env contain as store? nvidia or nvidia-api

jef · 2020-09-25T22:31:37Z

which should the .env contain as store? nvidia or nvidia-api

This would mean that you could use both. Either one. nvidia will scrape the site, the nvidia-api will call the APIs.

Note that nvidia is pretty iffy as their site doesn't like us scrapping it, so it'll start to timeout after so many requests. Your best best is just to use nvidia-api. Unless it breaks again due to Nvidia changing them.

manybot · 2020-09-25T22:33:17Z

which should the .env contain as store? nvidia or nvidia-api

This would mean that you could use both. Either one. nvidia will scrape the site, the nvidia-api will call the APIs

Thanks for the confirmation, to be on the safe side - ill use both ;)

I just wanted to make sure that somehow the way of use did not change 💯

Thanks for the great contribution @andrewmackrodt

niagi · 2020-09-25T22:33:38Z

Might as well leave the swedish FE3090 product ID here: 5438761600

jef · 2020-09-25T22:39:56Z

src/store/model/nvidia-api.ts

-	['spain', {drLocale: 'es_es', fe2060SuperId: null, fe3080Id: 5438794800, fe3090Id: null, nvidiaLocale: 'es_es'}],
-	['sweden', {drLocale: 'sv_SE', fe2060SuperId: null, fe3080Id: 5438798100, fe3090Id: null, nvidiaLocale: 'sv_se'}],
-	['usa', {drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: 5438481600, nvidiaLocale: 'en_us'}]
+	['austria', {currency: 'EUR', drLocale: 'de_de', fe2060SuperId: null, fe3080Id: 5440853700, fe3090Id: null, nvidiaLocale: 'de_de'}],


I wanted to remove one of these locales since it's only necessary we use one now, but I will update that in another chore PR.

I'd considered that, but this is a good point. I haven't tested whether adding to cart works for Canada and Poland which have different nvidiaLocale to drLocale. A shame the nvidia site is broken at the moment or I'd test this.

We'll clean it up if ends up being the case. This is solid for now though!

andrewmackrodt · 2020-09-25T22:40:19Z

@niagi I'd prefer if anything not directly related to changing the api implementation was a different PR. If nvidia change this implementation again and this PR gets reverted for some reason, then additional changes such as 3090 ids would also be lost.

seanogdev · 2020-09-25T23:10:29Z

Incredible work with both this & #270 @andrewmackrodt, @jef 👏

andrewmackrodt requested a review from jef as a code owner September 25, 2020 18:01

Fix nvidia-api

17d1aac

andrewmackrodt force-pushed the nvidia-api-fix branch from 4a26367 to 17d1aac Compare September 25, 2020 19:09

andrewmackrodt added 3 commits September 25, 2020 20:18

Update Poland currency code PLN

e7126f8

Fix Sweden currency code SEK

1b4a548

Fix currencies

7138d74

thegutuproject reviewed Sep 25, 2020

View reviewed changes

jef requested changes Sep 25, 2020

View reviewed changes

Modify getRegionInfo to reduce duplication

b24a080

jef requested changes Sep 25, 2020

View reviewed changes

Merge branch 'main' into nvidia-api-fix

4eba060

jef reviewed Sep 25, 2020

View reviewed changes

jef approved these changes Sep 25, 2020

View reviewed changes

jef changed the title ~~Fix nvidia-api~~ fix: nvidia-api Sep 25, 2020

jef merged commit 8d8e5b5 into jef:main Sep 25, 2020

andrewmackrodt deleted the nvidia-api-fix branch September 25, 2020 23:04

andrewmackrodt mentioned this pull request Sep 25, 2020

chore: remove nvidiaLocale #318

Merged

jef mentioned this pull request Sep 25, 2020

fix: fix Polish locale ID in nvidia-api #306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `nvidia-api` #314

fix: `nvidia-api` #314

andrewmackrodt commented Sep 25, 2020 •

edited

Loading

thegutuproject Sep 25, 2020

jef left a comment

jef Sep 25, 2020

andrewmackrodt Sep 25, 2020

jef Sep 25, 2020

jef Sep 25, 2020

jef Sep 25, 2020

andrewmackrodt Sep 25, 2020

jef Sep 25, 2020

jef Sep 25, 2020

andrewmackrodt Sep 25, 2020

jef Sep 25, 2020 •

edited

Loading

andrewmackrodt Sep 25, 2020

jef Sep 25, 2020

jef Sep 25, 2020

andrewmackrodt Sep 25, 2020

jef Sep 25, 2020

manybot commented Sep 25, 2020

jef commented Sep 25, 2020 •

edited

Loading

manybot commented Sep 25, 2020

niagi commented Sep 25, 2020

jef Sep 25, 2020

andrewmackrodt Sep 25, 2020

jef Sep 25, 2020

andrewmackrodt commented Sep 25, 2020

seanogdev commented Sep 25, 2020


		await page.setRequestInterception(true);

		page.on('request', interceptedRequest => {

	const defaultRegionInfo: NvidiaRegionInfo = {currency: 'USD', drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: null, nvidiaLocale: 'en_us'};
	const defaultRegionInfo: NvidiaRegionInfo = {currency: 'USD', drLocale: 'en_us', fe2060SuperId: 5379432500, fe3080Id: 5438481700, fe3090Id: 5438481600, nvidiaLocale: 'en_us'};

fix: nvidia-api #314

fix: nvidia-api #314

Conversation

andrewmackrodt commented Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

jef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jef Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manybot commented Sep 25, 2020

jef commented Sep 25, 2020 • edited Loading

manybot commented Sep 25, 2020

niagi commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewmackrodt commented Sep 25, 2020

seanogdev commented Sep 25, 2020

fix: `nvidia-api` #314

fix: `nvidia-api` #314

andrewmackrodt commented Sep 25, 2020 •

edited

Loading

jef Sep 25, 2020 •

edited

Loading

jef commented Sep 25, 2020 •

edited

Loading