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: Refactor notifications and make them injectable #2034

Closed
wants to merge 5 commits into from

Conversation

gmalette
Copy link
Contributor

Description

First I think it's worth giving context to what my goal is. I would like to create a program that wraps streetmerchant and offers another dashboard. I would like for it to run in the same process and avoid adding infrastructure if at all possible.

This PR refactors the notifications to make both failures and success notifications. It also makes the notification function injectable, but the main function uses the existing one.

I also refactored to make the logging of failures as part of normal notifications. This isn't strictly necessary but I thought it was nice to harmonize both and show that logging is just one way to handle poll events.

This PR, along with https://github.com/jef/streetmerchant/compare/main...gmalette:gm/packaging?expand=1, allow me to use streetmerchant and inject notification handling.

import {Browser, launch} from 'puppeteer';
import {config} from 'streetmerchant/build/src/config';
import {getSleepTime} from 'streetmerchant/build/src/util';
import {storeList} from 'streetmerchant/build/src/store/model';
import {tryLookupAndLoop} from 'streetmerchant/build/src/store';
import {LinkPollEvent} from 'streetmerchant/build/src/notification/link_poll_event';

const sendNotification = (event: LinkPollEvent): void => {
  console.log(event);
};

async function main() {
  const args: string[] = [];
  const browser: Browser = await launch({
    args,
    defaultViewport: {
      height: config.page.height,
      width: config.page.width,
    },
    headless: config.browser.isHeadless,
  });

  config.browser.userAgent = await browser.userAgent();

  for (const store of storeList.values()) {
    if (store.setupAction !== undefined) {
      store.setupAction(browser);
    }

    setTimeout(tryLookupAndLoop, getSleepTime(store), browser, store, sendNotification);
  }
}

void main();

Testing

Ran the program, worked as normal for "out of stock" and other errors.
I was not able to test for "in stock" items.

Guillaume Malette added 2 commits February 26, 2021 10:35
sendLogstream and link_poll_event have been moved to their respective
files
@gmalette gmalette requested a review from jef as a code owner February 26, 2021 16:38
getSleepTime(store),
browser,
store,
sendNotification
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the main function we inject the existing sendNotification function

export type LinkPollEvent = (LinkPollError | LinkPollSuccess) & {
link: Link;
store: Store;
};
Copy link
Contributor Author

@gmalette gmalette Feb 26, 2021

Choose a reason for hiding this comment

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

The LinkPollEvent could be made shorter but I liked to express it in a way that prevents representing illegal states.

I wasn't sure if events should be provided for cloudflare, tryLookupAndLoop caught errors, inStockWaiting, lookupCard caught errors, NoResponse, RateLimit, etc.

assertNever(pollEvent);
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the logging w.r.t polling had been moved here. Some logging still happends in lookup.ts but looked ancillary, except for what's mentioned in the comments on notifications.

async function lookup(
browser: Browser,
store: Store,
sendNotification: SendNotification
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the added code in this file is passing the sendNotification to the functions that need it.

The rest is sending notifications in lieu of logging

logger.info(Print.maxPrice(link, store, pollEvent.maxPrice, true));
return;
case 'request_failed':
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this case logs too but I haven't found where it happens.

@jef
Copy link
Owner

jef commented Feb 27, 2021

I'll be looking at this and #2038 very shortly.

I appreciate your contribution!

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.

Looks good! I'm not 100% certain if folks want emails and such everytime captcha, out of stock, and etc show up.

I do understand the concept, but I'm not certain if we want this behavior.

That being said, if you make it configurable, I'll be more than happy to pull it in!

Comment on lines 50 to 52
const {link, store} = pollEvent;
if (pollEvent.result === 'failure') {
switch (pollEvent.failureReason) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const {link, store} = pollEvent;
if (pollEvent.result === 'failure') {
switch (pollEvent.failureReason) {
const {failureReason, link, result, store} = pollEvent;
if (result === 'failure') {
switch (failureReason) {

Might as well yank these out as well.

@gmalette
Copy link
Contributor Author

gmalette commented Mar 3, 2021

@jef I'm not sure I understand what kind of changes you're asking for. The default injected sendNotification will stop after sending the logstream event unless it's a success, meaning that the existing behaviour should remain unaltered.

@gmalette
Copy link
Contributor Author

gmalette commented Mar 9, 2021

@jef can you clarify what you were looking for me to change?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2021

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@jef
Copy link
Owner

jef commented Apr 9, 2021

Sorry for the late response @gmalette, this have been pretty busy on my end.

Let me take another look at this shortly and we can get this in.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2021

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days

@github-actions
Copy link
Contributor

This issue has been closed because it is stale. Reopen if necessary.

@github-actions github-actions bot closed this Jun 19, 2021
@gmalette gmalette deleted the gm/notifications branch July 26, 2021 00:37
@gmalette
Copy link
Contributor Author

LMK if you'd like me to reopen this, it would probably help with #2337, especially now that we have #2066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants