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

fix issue #124 #125

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

fix issue #124 #125

wants to merge 11 commits into from

Conversation

lookfirst
Copy link
Contributor

No description provided.

@plibither8
Copy link
Owner

Woah, thanks so much for working on this issue! Taking a look...

@lookfirst
Copy link
Contributor Author

No problem. It is a hacky solution, but it is what it is.

What I'd do is add a cache somewhere so that common selectors like this aren't done multiple times in multiple places... I assume the browser has this cache internally, but it would still be good to add a layer above it for further optimization.

@plibither8
Copy link
Owner

We can look into adding a session-cache that is available for the current page and refreshes on page-change or reload. I believe persisting such a cache over different page loads can cause problems when the underlying HTML (i.e. the items/comments) change. What do you think?

@lookfirst
Copy link
Contributor Author

Just a simple in memory LRU cache that flushes on reload.

But, this is a larger architectural issue. I would have designed things a bit differently from the start. Primarily, I'd have a wrapper around document.querySelector() so that I'd have more control over errors as well. Instead of just breaking the entire page and a console message. I think it would be nicer to catch null responses and deal with them more elegantly (ie: a nice message to the user). The caching layer could be embedded in that too.

If/when this thing converts to typescript, I'd rewrite large portions from the ground up.

@lookfirst
Copy link
Contributor Author

Here is another thing I was just thinking about... think of the page as a database full of data that you want to action on. You'd want to effectively have a data layer that you work against. Right now, you're just plucking what you need from the dom, but instead... map the whole page into an object structure and then you can work against that API instead of just randomly finding stuff in the page.

@lookfirst
Copy link
Contributor Author

Then, instead of a cache on individual queries, you just CRUD portions of your data structure.

@plibither8
Copy link
Owner

You just penned down my exact thoughts, pretty much what I was thinking of doing since a long time. As you mentioned, the extension requires a pretty large refactor, and that requires time. My semester at uni will be ending around mid-December, so hope to work on this then! Will definitely loop you in the architectural decisions, and if you can and are willing to, we can work on it together. 😄

Until then, I'll take a look at this PR and move forward with it. Thanks again!

@lookfirst
Copy link
Contributor Author

@plibither8 ...

@lookfirst
Copy link
Contributor Author

@plibither8 ... adding some more fixes...

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.

None yet

2 participants