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 missing documention + env variables, add country attribute to stores, fix linting errors, deprecate Twitch notification #3198

Merged
merged 11 commits into from
Jan 30, 2025

Conversation

alanbixby
Copy link
Contributor

@alanbixby alanbixby commented Jan 29, 2025

Description

While setting up my streetmerchant client ahead of tomorrow's 5090 launch, I noticed the documentation had fallen out of sync over the years and linting rules were not being enforced. As part of these changes, I ran an extract to update the list of models per series, add notify groups to missing products like the A3 mATX Lian Li case, and fixed some type errors + linting errors.

All of the models + series were code gen'd based on the exports in src/store/model and cross referenced with the new models in #3191 . As a future enhancement, this probably should be procedural and moved into the CI/CD pipeline - but that's out of scope for this PR.

Regarding linting rules, I have also disabled the linting rule @typescript-eslint/no-explicit-any - there library currently has ~40 linter warnings for this rule - all of which I'd consider acceptable uses of any.

--

⚠ Deprecation!

While testing with the main branch, I found the Twitch notification client was firing (despite blank environment variables!) on any product match and threw a runtime error on any product hit.

var _this = _super.call(this) || this;
   TypeError: Class constructor EventEmitter cannot be invoked without 'new'
    at ChatClient.IrcClient [as constructor] (/streetmerchant/node_modules/.pnpm/[email protected]/node_modules/ircv3/lib/IrcClient.js:24:28)
    at new ChatClient (/streetmerchant/node_modules/.pnpm/[email protected][email protected]/node_modules/twitch-chat-client/lib/ChatClient.js:44:28)
    at Object.<anonymous> (/streetmerchant/build/src/messaging/twitch.js:23:20)

This is an error from an upstream dependency via twitch-chat-client - these packages are deprecated in favor of @twurple/chat and @twurple/auth - but given the timeline and my lack of experience with these libraries I opted to disable the functionality and add a warning in config.ts for users who provide a Twitch credentials. Given the lack of issues + the recently merged PRs, it is entirely possible this is an issue isolated to my environment, if that's the case let me know and I can revert that portion of the PR.

--

I have also laid the groundwork for a SHOW_ONLY_COUNTRY filter. Implementation is not complete, but attributes have been added to all of the store objects, and a getAllCountries() function was written - I just haven't wrapped my head around how the web UI pushes updates or a clean place to integrate the filter in this PR.

--

All edits to the imports are transpose operations from sort import rules being applied. No dependencies are changing outside of the deprecation of twitch, twitch-chat-client, and twitch-auth.

Testing

I have ran npm run test, npm run test:notification with no errors and ran in production mode with a config tracking the new products successfully.

@alanbixby alanbixby requested a review from jef as a code owner January 29, 2025 23:02
@alanbixby
Copy link
Contributor Author

*another breaking change, this changes MAX_PRICE_SERIES_RYZEN9800XX3D to MAX_PRICE_SERIES_RYZEN9800X3D (one x)

This variable was inconsistent with DISCORD_NOTIFY_GROUP_RYZEN9800X3D and is a misspelling of the product name.

jef
jef previously approved these changes Jan 29, 2025
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.

Country code is SMART! I love it and the implementation and the other chores around the repo.

I agree with you on the documentation, that would be a great thing to implement -- self-documenting :)

I agree with the any check. I think there was a time I wanted to be strict and have all the types specified, but with my contribution level and making this more accessible for contribution, I think ignoring that rule is that proper thing to do.

I really appreciate your time and effort on this one. Looking forward to thing going in and making this project great! 🚀🚀🚀

@alanbixby
Copy link
Contributor Author

alanbixby commented Jan 30, 2025

FYI - I have another PR on the way adding https://us-store.msi.com/ EDIT: ready in #3199

I didn't realize there were substantive changes within the last 5 hours for #3191 - going to be pushing another set of label and type fixes for this ticket to reflect them as we are missing prime and vanguard

@alanbixby
Copy link
Contributor Author

Ready for final review and merge.

@jef jef merged commit c2ed997 into jef:main Jan 30, 2025
3 checks passed
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.

2 participants