Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

Basic Issue List Implementation with localStorage caching #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeffbcross
Copy link
Contributor

No description provided.

@addyosmani
Copy link

Very minor comment: is there a reason localStorage is preferred over IDB here? localForage offers a ~similar async alternative to localStorage using IDB as backing storage and falls back to LS otherwise. Feel free to ignore :)

@jeffbcross
Copy link
Contributor Author

@addyosmani thanks for taking a look. I wanted to get a rudimentary caching implementation here just to get some basic caching going. IDB should definitely be the store, and it will be treated as the source of truth with some kind of synchronization mechanism.

The store will be more structured than key/value, so localForage probably wouldn't be the best fit. @robwormald has started an IndexedDB implementation based on rxjs at ngrx/db that hopefully can be used here, but I'm not sure what shape it's in :).

@jeffbcross
Copy link
Contributor Author

Pushed updated PR, added some todos, added some tests, made the github service get the auth token itself instead of having it passed in.

this.repos
.map((repos:Repo[]) => repos[0])
.take(1)
.subscribe(repo => this.repoSelection.next(repo));
Copy link
Member

Choose a reason for hiding this comment

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

.subscribe(repoSelection);

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'd bet that internal Subject logic references this at some point, which would cause an unbounded call to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, no you're right, I was thinking of .next(), but it's an observer, it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jeffbcross
Copy link
Contributor Author

K thx

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

Successfully merging this pull request may close these issues.

3 participants