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

Integrate PouchDB #1167

Closed
wants to merge 52 commits into from
Closed

Integrate PouchDB #1167

wants to merge 52 commits into from

Conversation

midsorbet
Copy link
Contributor

@midsorbet midsorbet commented May 30, 2018

To test this PR, please make sure to create a database called "test" and add "pouchIndex": "courses" to the documents in the courses database. (related to #48)

What Works?

  • Authentication (Sign up, login)
  • Courses module (only the courses services use pouch)

@@ -26,7 +27,7 @@ export class CoursesService {
// If the id already matches what is stored on the service, return that.
// Or will get new version if forceLatest set to true
requestCourse({ courseId, forceLatest = false }, opts: any = {}) {
if (!forceLatest && this.course && courseId === this.course._id) {
if (!forceLatest && courseId === this.course._id) {
Copy link
Contributor Author

@midsorbet midsorbet Jun 5, 2018

Choose a reason for hiding this comment

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

So, I added a check for this.course because it's null sometimes and gives an error when accessing the id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, if you directly go to the step view page then BehaviorSubject is null and throws an error

@midsorbet midsorbet changed the title [WIP] Integrate PouchDB Integrate PouchDB Jun 7, 2018
@midsorbet midsorbet removed the WIP label Jun 7, 2018
@paulbert
Copy link
Member

Ok here are some thoughts on this:

  • I think we should not start with courses on this. This is actively in development and changing weekly, making the conflicts pile up quickly. I think feedback would be best.
  • Before doing that, maybe let's just add in the PouchDB service into the codebase rather than try to integrate it fully into a section of the app. I know that has its own challenges, but there's so much changing here that it'll be easier to progress with a small change adding PouchDB
  • The folder structure is adding an unnecessary layer (shared/services) when I think we should just have the couch and pouch services in a shared/database folder or even just a database folder.
  • It's also easier to follow the changes if each commit isn't so specific (a few that I looked at were one line changes). We don't want to get too broad, but something in the middle would improve this.
  • Thinking about it more, I think we need have separate PRs for adding interfaces. This will also help to reduce the size of this PR and get it moving faster.

@midsorbet
Copy link
Contributor Author

Thanks for the feedback @paulbert. Closing in favor of #1355

@midsorbet midsorbet closed this Jun 14, 2018
@dogi dogi removed the in progress label Jun 14, 2018
@midsorbet midsorbet deleted the pouch branch June 28, 2018 18:17
@midsorbet midsorbet restored the pouch branch June 28, 2018 18:20
@paulbert paulbert deleted the pouch branch October 25, 2018 05:24
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

4 participants