emit correct add/update events and emit event before resolving promises #154

gr2m · 2017-06-26T01:48:41Z

This is a workaround for the PouchDB feature request at pouchdb/pouchdb#6552

gr2m · 2017-07-11T18:16:02Z

lib/reset.js

@@ -11,7 +11,7 @@ function reset (state) {
  })

  .then(function () {
-    return state.db.destroy
+    return state.db.destroy()


ha! Thanks Rob :)

You're most welcome, Gregor. If I had a dollar every time I did that, I'd be retired already! :)

capellini · 2017-07-12T03:27:05Z

@gr2m -- I'm trying to rebase master and refactor the tests to get back to 100% code coverage.

Should we be resolving somewhere in here? I see where we do it when result.length == 0, but otherwise, it doesn't look like we resolve.

gr2m · 2017-07-13T03:48:33Z

thanks for looking into it 👍

We set scope.resolve on line 18. The resolve happens in changeHandler in line 85. That way we make sure that all change events happen before the promise gets resolved.

Does that make sense?

capellini · 2017-07-13T22:22:02Z

Does that make sense?

Ah, I see it now. Let me see if I can figure out the problem with the test.

capellini · 2017-07-14T01:04:46Z

@gr2m -- I'm almost there. I figured out the db-bulk-docs.js test, but I forgot that I had a few more statements that I needed to cover for handle-changes.js. Specifically, I have to test (1) when isBootstrapping is true here and (2) when we are trying to delete an unknown doc id.

Any ideas on the best ways to approach these tests?

capellini · 2017-07-14T01:15:08Z

@gr2m -- looks like the travis builds are failing because pouchdb-utils needs the uuid/v4 package, but never specifies that in its package.json? Is that what you're seeing?

gr2m · 2017-07-14T20:55:06Z

checking. They just did a release, it might be related to that

Update: yep: pouchdb/pouchdb#6613

gr2m · 2017-07-14T22:47:04Z

I’ve restarted the builds, I think the problem got fixed by another patch release.

I also started a PR that fixes the versions for pouchdb-utils and pouchdb-errors as it is recommended in their readmes. PR is here: #161

capellini · 2017-07-14T23:39:22Z

Ok, looks like we're back! 🙌 I could just use your input on how to cover the lines below. In the meantime, I'll keep plugging away to see if I can figure it out.

@gr2m -- I'm almost there. I figured out the db-bulk-docs.js test, but I forgot that I had a few more statements that I needed to cover for handle-changes.js. Specifically, I have to test (1) when isBootstrapping is true here and (2) when we are trying to delete an unknown doc id.

Any ideas on the best ways to approach these tests?

gr2m · 2017-07-16T19:45:42Z

@capellini

(1) when isBootstrapping is true here

I would test it with a unit test. Pass in state with

spy on state.emitter.emit
db.allDocs() returning a pending promise
make state.db.changes() return a mock with a spy on .on so we get access the callback passed to .on('change', callback).
run the callback with a dummy change
now resolve the promise returned by db.allDocs() with {rows: []} and check if state.emitter.emit was called correctly with they dummy change

(2) when we are trying to delete an unknown doc id.

Here I think we could do an integration test. Create the remote database with PouchDB, create a document init, then delete it. Now initialise Store using the existing database as remote and run store.sync(). It should not emit any change or remove events.

Let me know if that works :)

capellini · 2017-07-18T04:27:01Z

Let me know if that works :)

Worked perfectly! Thanks for your help!

I increased the code coverage to 100% and rebased to a single test and fix commit each. Please let me know what you think when you get a chance. 🙂

gr2m

This is really good work! I just have a few comments :) Let me know if I can help with anything

gr2m · 2017-07-18T04:34:14Z

tests/utils/pouchdb.js

+
+// module.exports = require('pouchdb-monorepo/packages/node_modules/pouchdb-core')
+  // .plugin(require('pouchdb-monorepo/packages/node_modules/pouchdb-replication'))
+  // .plugin(require('pouchdb-monorepo/packages/node_modules/pouchdb-adapter-memory'))


I guess these are remains from debugging?

Yes, I was wondering about that, too. I should have removed it. Sorry about that!

gr2m · 2017-07-18T04:35:05Z

lib/with-id-prefix.js

@@ -3,7 +3,7 @@ module.exports = withIdPrefix
 var EventEmitter = require('events').EventEmitter

 /**
- * returns API with all CRUD methods scoped to id prefix
+ * returns API with all CRUD methods scoped to id prefix`


can you remove the backtick?

gr2m · 2017-07-18T04:36:52Z

tests/integration/events.js

@@ -388,6 +390,28 @@ test('store.on("change") with adding one', function (t) {
  })
 })

+test('store.on("add") should emit after store.add() promise resolved', function (t) {


I think this should say:
store.on("add") should not emit after store.add() promise resolved

gr2m · 2017-07-18T04:51:04Z

tests/unit/handle-changes-test.js

+
+  handleChanges(state)
+
+  Promise.resolve(state.bootstrap)


I'm not sure what you do here exactly :) state.bootstrap is undefined, so you this is basically a noop

What I meant in my comment was to do something like this

var resolveAllDocs state.db.allDocs = new Promise(function (resolve) { resolveAllDocs = resolve })

That way you can resolve db.allDocs() at any point you like. Now you can emit the events and then call resolveAllDocs().

I think the current est works because handleChanges runs all synchronous, so the callback passed to state.db.changes().on('change', callback) gets executed immediately, while simple.spy().resolveWith({ rows: [] }) resolves on next tick.

state.bootstrap is undefined

Actually, state.bootstrap is a Promise, per the assignment here. What happens is, handleChange runs synchronously and stores the pending Promise from state.db.allDocs().then() call into state.bootstrap, so state.bootstrap is actually a Promise.

But I see your point -- there's nothing to prevent resolving the .then() call chain after the state.db.allDocs(), which we want to control the flow of. I'll work on refactoring that. 👍

gr2m · 2017-07-18T04:53:51Z

tests/unit/handle-changes-test.js

+  var state = {
+    db: {
+      allDocs: simple.spy().resolveWith({ rows: [] }),
+      changes: simple.spy().returnWith(changesSpy)


I’m surprised that simple.spy() works here :) I think using simple.mock() would make more sense, a .spy() is usually used to count how often a method was called without altering its behaviour, while a .mock() can change the behavior, too. I guess both work in this case, but I’d prefer simple.mock()

Here is an example of what I mean: https://runkit.com/embed/2gcmr9w1xhjs

I think that's what I like best about simple-mock -- they know that the dividing lines between mock, stubs, spies, etc is contentious and so they don't provide much of an opinion on the matter. It just works! 😄

But you're right -- they should be mocks.

capellini · 2017-07-18T12:55:03Z

I tried this:

var resolveAllDocs
state.db.allDocs = new Promise(function (resolve) {
  resolveAllDocs = resolve
})

but it didn't work because state.db.allDocs() is not a function. So I tried variants of things like this:

var resolveAllDocs
state.db.allDocs = function () {
  return new Promise(function (resolve) {
    resolveAllDocs = resolve
  })
}

but that didn't work either, as the .then(...) after the allDocs() call was not being intercepted, so eventually I settled on coming up with my own fake promise like this:

simple.mock(state.db, 'allDocs').returnWith({
  then: simple.spy().callFn(function (callback) {
    handleResults = callback
  })
})

Please let me know if there's a better way to do this and thanks so much for your help!

p.s. I noticed that the travis build is failing, but it's not obvious to me why. ☹️ Is there a way that I can restart the build (without doing an empty commit/push)?

capellini · 2017-07-18T13:14:01Z

I noticed that the travis build is failing, but it's not obvious to me why. ☹️ Is there a way that I can restart the build (without doing an empty commit/push)?

Actually, I found another typo that I could fix, so I was able to do another commit. 🙂

But for future reference, is there a way to restart a travis build, or is that something that only the travis account owner can do?

gr2m · 2017-07-18T15:49:16Z

it didn't work because state.db.allDocs() is not a function

I’m sorry, it was late yesterday and I didn’t try it out myself. Try this

var resolveAllDocs
var promise = new Promise(function (resolve) {
  resolveAllDocs = resolve
})
state.db.allDocs = function () { return promise }

That should do. This is the so called "deferred pattern" which is an anti pattern, but I don’t have a better idea, and it’s only a test, not implementation, so we should be fine :D

capellini · 2017-07-18T21:39:35Z

@gr2m - ok, I think I've got it now. Please let me know what you think. Thanks!

gr2m · 2017-07-18T22:10:45Z

Okay this looks good now, great work, thanks Rob!

capellini · 2017-07-18T22:54:27Z

Thanks, Gregor! It would be disingenuous to take much of the credit, though, as you did most of the hard work! 🙂

gr2m force-pushed the add-update-events-from-remote branch from 7be4e39 to 8c765b4 Compare June 28, 2017 02:17

gr2m changed the base branch from hoodiehq/camp#121 to master June 28, 2017 02:17

gr2m force-pushed the add-update-events-from-remote branch from 8c765b4 to 9f80b73 Compare June 28, 2017 02:25

gr2m force-pushed the add-update-events-from-remote branch from 9f80b73 to d5b29c0 Compare July 5, 2017 05:16

gr2m commented Jul 11, 2017

View reviewed changes

capellini force-pushed the add-update-events-from-remote branch from 0184060 to 518b4e3 Compare July 14, 2017 00:59

capellini force-pushed the add-update-events-from-remote branch from cfc37e8 to c381cf8 Compare July 14, 2017 23:30

capellini force-pushed the add-update-events-from-remote branch from 53946dd to 56a4351 Compare July 18, 2017 04:12

gr2m commented Jul 18, 2017

View reviewed changes

capellini force-pushed the add-update-events-from-remote branch 2 times, most recently from 9c0b592 to 1adb0c7 Compare July 18, 2017 12:42

capellini force-pushed the add-update-events-from-remote branch from 1adb0c7 to 44b00b0 Compare July 18, 2017 13:06

gr2m added 2 commits July 18, 2017 16:00

test: add/update events based on doc existence

596c734

fix: add/update events based on doc existence in db

240b61b

capellini force-pushed the add-update-events-from-remote branch from 44b00b0 to 240b61b Compare July 18, 2017 20:00

gr2m merged commit 4cd0b26 into master Jul 18, 2017

gr2m deleted the add-update-events-from-remote branch July 18, 2017 22:54

gr2m mentioned this pull request Jul 19, 2017

performance impact of #154 #162

Open

gr2m mentioned this pull request Jul 30, 2017

correctly cleanup reset listener #164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emit correct add/update events and emit event before resolving promises #154

emit correct add/update events and emit event before resolving promises #154

gr2m commented Jun 26, 2017 •

edited

Loading

gr2m Jul 11, 2017

capellini Jul 11, 2017

capellini commented Jul 12, 2017

gr2m commented Jul 13, 2017

capellini commented Jul 13, 2017

capellini commented Jul 14, 2017

capellini commented Jul 14, 2017

gr2m commented Jul 14, 2017 •

edited

Loading

gr2m commented Jul 14, 2017

capellini commented Jul 14, 2017

gr2m commented Jul 16, 2017

capellini commented Jul 18, 2017

gr2m left a comment

gr2m Jul 18, 2017

capellini Jul 18, 2017

gr2m Jul 18, 2017

gr2m Jul 18, 2017

gr2m Jul 18, 2017

capellini Jul 18, 2017

gr2m Jul 18, 2017 •

edited

Loading

capellini Jul 18, 2017

capellini commented Jul 18, 2017 •

edited

Loading

capellini commented Jul 18, 2017 •

edited

Loading

gr2m commented Jul 18, 2017

capellini commented Jul 18, 2017

gr2m commented Jul 18, 2017

capellini commented Jul 18, 2017 •

edited

Loading

emit correct add/update events and emit event before resolving promises #154

emit correct add/update events and emit event before resolving promises #154

Conversation

gr2m commented Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capellini commented Jul 12, 2017

gr2m commented Jul 13, 2017

capellini commented Jul 13, 2017

capellini commented Jul 14, 2017

capellini commented Jul 14, 2017

gr2m commented Jul 14, 2017 • edited Loading

gr2m commented Jul 14, 2017

capellini commented Jul 14, 2017

gr2m commented Jul 16, 2017

capellini commented Jul 18, 2017

gr2m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capellini commented Jul 18, 2017 • edited Loading

capellini commented Jul 18, 2017 • edited Loading

gr2m commented Jul 18, 2017

capellini commented Jul 18, 2017

gr2m commented Jul 18, 2017

capellini commented Jul 18, 2017 • edited Loading

gr2m commented Jun 26, 2017 •

edited

Loading

gr2m commented Jul 14, 2017 •

edited

Loading

gr2m Jul 18, 2017 •

edited

Loading

capellini commented Jul 18, 2017 •

edited

Loading

capellini commented Jul 18, 2017 •

edited

Loading

capellini commented Jul 18, 2017 •

edited

Loading