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

feat: add redis #1390

Merged
merged 17 commits into from
Dec 26, 2020
Merged

feat: add redis #1390

merged 17 commits into from
Dec 26, 2020

Conversation

BaileyMillerSSI
Copy link

@BaileyMillerSSI BaileyMillerSSI commented Dec 13, 2020

Description

Adds Redis as a way to keep track of stock. Redis connection string can be passed in to meet many different configurations.

Testing

  1. Startup redis (I used docker) with the default port
  2. Setup your .env file to set the url (REDIS_URL="redis://localhost:6379/1")
  3. Run the test command and verify that redis has a key in it.

New dependencies

redis
@types/redis

@BaileyMillerSSI BaileyMillerSSI requested a review from jef as a code owner December 13, 2020 21:26
@jef
Copy link
Owner

jef commented Dec 13, 2020

Great! This will be useful for things coming up in 4.0.0 as well.

Thank you!

@BaileyMillerSSI
Copy link
Author

@jef Of course! I am fixing these CI errors and I am going to add more options to pass into the redis client.

@BaileyMillerSSI
Copy link
Author

@jef
I am having trouble getting the eslint step to pass. Are you able to provide any feedback on that? I cannot seem to get eslint to auto fix the problems so I am not sure how to fix some of the errors.

@jef
Copy link
Owner

jef commented Dec 13, 2020

@jef
I am having trouble getting the eslint step to pass. Are you able to provide any feedback on that? I cannot seem to get eslint to auto fix the problems so I am not sure how to fix some of the errors.

Haha, I'm sorry. I can be hell sometimes. Let's take a look when it's done building.

Your best bet is usually running npm run lint:fix and it should fix most of the issues for you, except the import sorting.


Looks like it's good now! Thanks for taking the time to fix that 😅 Sorry about it.

@BaileyMillerSSI
Copy link
Author

@jef
I am having trouble getting the eslint step to pass. Are you able to provide any feedback on that? I cannot seem to get eslint to auto fix the problems so I am not sure how to fix some of the errors.

Haha, I'm sorry. I can be hell sometimes. Let's take a look when it's done building.

I think I got it this time, I am using the prettier extension for VS Code but some of the auto formats are against the CI rules but I think I managed to figure it out through trial and error.

@BaileyMillerSSI
Copy link
Author

BaileyMillerSSI commented Dec 13, 2020

@jef
I think I am done implementing this, please feel free to review this whenever you get a chance.

I would really like to figure out how to have this run on every pass. Meaning this currently only updates Redis if the notification gets triggered. I would like Redis to be updated every time a page is loaded and it's either in/out of stock.

I am currently letting this run for a bit so I can see how it looks once something comes into stock.

jef
jef previously approved these changes Dec 26, 2020
Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

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

Awesome! I love it. We should add this to the docker-compose sometime. Could be cool.

Thanks!

@jef jef enabled auto-merge (squash) December 26, 2020 00:13
@jef jef changed the title feat: Implements Redis key-value-store feat: add redis Dec 26, 2020
@gigi2006
Copy link

Windows User Need redis Server start
I have fix this for me Whit this https://github.com/microsoftarchive/redis/releases/latest

@jef
Copy link
Owner

jef commented Dec 26, 2020

Made a fix in the latest commits to not have it required.

Thank you!

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.

3 participants