chris-mosley / AmazonBrandFilter

Filters Amazon search results to only be "known" brands.
MIT License
44 stars 5 forks source link

Improved personal brands filter that works in real time without a page reload #35

Closed barrymun closed 11 months ago

barrymun commented 11 months ago
barrymun commented 11 months ago

@chris-mosley let me know what you think of this one when you have time, thanks!

two things to note here:

  1. i noticed that a package-lock.json file had been added so i removed it. we should stick to a single package manager, otherwise we might get conflicts in the future. i added the run-script-os lib to package.json to it will be included in future installs. for now it would be easier to stick with yarn, unless there is a specific reason to switch to npm?
  2. there are many cases where using sync when getting/setting storage values may return an empty object, so i've added a fallback where we will also set in local, and if nothing is found when retrieving using sync we just use local instead. the question is: should i make this the default behaviour for everything (i.e., in getStorageValue should i check local if nothing found for sync, and in setStorageValue should i always set for local too even when sync is specified)?
chris-mosley commented 11 months ago

for now it would be easier to stick with yarn, unless there is a specific reason to switch to npm?

This is an artifact of an SRE guy trying to keep up with a front-end guy on what has become front-end code.

For 2. Basically by default I want all configuration settings in sync except for the brands list itself. It makes sense to me that users would want to have the settings consistent between all of their devices.

I'm checking the PR now.

chris-mosley commented 11 months ago

@barrymun So the update you made to the brand search seems to be pretty broken. On firefox it's failing to find a lot of brands and filtering out almost all results.

It seems to work well in chrome though.

In the future please test both browsers more thoroughly before creating a PR

EDIT: Further testing shows this is an artifact of the personal list. It seems to function normally with the personal list disabled

barrymun commented 11 months ago

for now it would be easier to stick with yarn, unless there is a specific reason to switch to npm?

This is an artifact of an SRE guy trying to keep up with a front-end guy on what has become front-end code.

For 2. Basically by default I want all configuration settings in sync except for the brands list itself. It makes sense to me that users would want to have the settings consistent between all of their devices.

I'm checking the PR now.

no problem, npm can be used instead of yarn, but i was just wondering as we were already using yarn.

ok i'll put together a pr which uses sync for everything (except for the brands), but will auto fall back to using local where sync data is not available (e.g., user not authenticated on chrome)

barrymun commented 11 months ago

@barrymun So the update you made to the brand search seems to be pretty broken. On firefox it's failing to find a lot of brands and filtering out almost all results.

It seems to work well in chrome though.

In the future please test both browsers more thoroughly before creating a PR

EDIT: Further testing shows this is an artifact of the personal list. It seems to function normally with the personal list disabled

@chris-mosley thanks for testing this. is there a problem with the firefox addon (i always test both builds before creating a pr)? if so let me know (and how to recreate) please and i'll try to fix it

chris-mosley commented 11 months ago

My guess is different regex interpreters.

In Firefox when you enable the personal list it will clear out pretty much every result. That's even when the list is empty, adding any entries to it doesn't seem to make much difference.

Thinking back this is exactly the reason i added debug mode but I'm not in a position to look at it again until later tonight.

barrymun commented 11 months ago

My guess is different regex interpreters.

In Firefox when you enable the personal list it will clear out pretty much every result. That's even when the list is empty, adding any entries to it doesn't seem to make much difference.

Thinking back this is exactly the reason i added debug mode but I'm not in a position to look at it again until later tonight.

i'll investigate and try to recreate

chris-mosley commented 11 months ago

for now it would be easier to stick with yarn, unless there is a specific reason to switch to npm?

This is an artifact of an SRE guy trying to keep up with a front-end guy on what has become front-end code. For 2. Basically by default I want all configuration settings in sync except for the brands list itself. It makes sense to me that users would want to have the settings consistent between all of their devices. I'm checking the PR now.

no problem, npm can be used instead of yarn, but i was just wondering as we were already using yarn.

ok i'll put together a pr which uses sync for everything (except for the brands), but will auto fall back to using local where sync data is not available (e.g., user not authenticated on chrome)

Yeah I'm fine with yarn. I honestly didn't even know yarn was a different package manager to npm. I thought it was a wrapper that leveraged npm or something like that. I just looked into how to get the package working and in my stumbling apparently got npn working, but i have been using yarn on my box.