Breaking: remove deferred chained batch (#51)

README.md

@@ -350,16 +350,22 @@ await db.batch([
 
 ### `chainedBatch = db.batch()`
 
-Create a [chained batch](#chainedbatch), when `batch()` is called with zero arguments. A chained batch can be used to build and eventually commit an atomic batch of operations. Depending on how it's used, it is possible to obtain greater performance with this form of `batch()`. On several implementations however, it is just sugar.
+Create a [chained batch](#chainedbatch), when `batch()` is called with zero arguments. A chained batch can be used to build and eventually commit an atomic batch of operations:
 
 ```js
-await db.batch()
+const chainedBatch = db.batch()
   .del('bob')
   .put('alice', 361)
   .put('kim', 220)
-  .write()
+
+// Commit
+await chainedBatch.write()
 ```
 
+Depending on how it's used, it is possible to obtain greater overall performance with this form of `batch()`, mainly because its methods like `put()` can immediately copy the data of that singular operation to the underlying storage, rather than having to block the event loop while copying the data of multiple operations. However, on several `abstract-level` implementations, chained batch is just sugar and has no performance benefits.
+
+Due to its synchronous nature, it is not possible to create a chained batch before the database has finished opening. Be sure to call `await db.open()` before `chainedBatch = db.batch()`. This does not apply to other database methods.
+
 ### `iterator = db.iterator([options])`
 
 Create an [iterator](#iterator). The optional `options` object may contain the following _range options_ to control the range of entries to be iterated:

UPGRADING.md

@@ -168,6 +168,36 @@ indexes.batch([{ type: 'del', key: 'blue', sublevel: colorIndex }])
 db.batch([{ type: 'del', key: 'blue', sublevel: colorIndex }])
 ```
 
+#### 1.5. Open before creating a chained batch
+
+It is no longer possible to create a chained batch while the database is opening. If you previously did:
+
+```js
+const db = new ExampleLevel()
+
+const batch = db.batch().del('example')
+await batch.write()
+```
+
+You must now do:
+
+```js
+const db = new ExampleLevel()
+await db.open()
+
+const batch = db.batch().del('example')
+await batch.write()
+```
+
+Alternatively:
+
+```js
+const db = new ExampleLevel()
+await db.batch([{ type: 'del', key: 'example' }])
+```
+
+As for why that last example works yet the same is not supported on a chained batch: the `put()`, `del()` and `clear()` methods of a chained batch are synchronous. This meant `abstract-level` (and `levelup` before it) had to jump through several hoops to make it work while the database is opening. Having such logic internally is fine, but the problem extended to the new [hooks](./README.md#hooks) feature and more specifically, the `prewrite` hook that runs on `put()` and `del()`.
+
 ### 2. Private API
 
 #### 2.1. Promises all the way

abstract-chained-batch.js

@@ -265,13 +265,10 @@ class AbstractChainedBatch {
   _clear () {}
 
   async write (options) {
+    assertStatus(this)
     options = getOptions(options)
 
-    if (this[kStatus] !== 'open') {
-      throw new ModuleError('Batch is not open: cannot call write() after write() or close()', {
-        code: 'LEVEL_BATCH_NOT_OPEN'
-      })
-    } else if (this[kLength] === 0) {
+    if (this[kLength] === 0) {
       return this.close()
     } else {
       this[kStatus] = 'writing'
@@ -391,9 +388,12 @@ function assertStatus (batch) {
     })
   }
 
-  // TODO (next major): enforce this regardless of hooks
-  if (batch[kPrewriteBatch] !== null && batch.db.status !== 'open') {
-    throw new ModuleError('Chained batch is not available until database is open', {
+  // Can technically be removed, because it's no longer possible to call db.batch() when
+  // status is not 'open', and db.close() closes the batch. Keep for now, in case of
+  // unforseen userland behaviors.
+  if (batch.db.status !== 'open') {
+    /* istanbul ignore next */
+    throw new ModuleError('Database is not open', {
       code: 'LEVEL_DATABASE_NOT_OPEN'
     })
   }

abstract-level.js

@@ -526,7 +526,6 @@ class AbstractLevel extends EventEmitter {
   // of classic-level, that should not be copied to individual operations.
   batch (operations, options) {
     if (!arguments.length) {
-      if (this[kStatus] === 'opening') return new DefaultChainedBatch(this)
       assertOpen(this)
       return this._chainedBatch()
     }

lib/default-chained-batch.js

@@ -1,10 +1,9 @@
 'use strict'
 
 const { AbstractChainedBatch } = require('../abstract-chained-batch')
-const ModuleError = require('module-error')
 const kEncoded = Symbol('encoded')
 
-// Functional default for chained batch, with support of deferred open
+// Functional default for chained batch
 class DefaultChainedBatch extends AbstractChainedBatch {
   constructor (db) {
     // Opt-in to _add() instead of _put() and _del()
@@ -21,19 +20,9 @@ class DefaultChainedBatch extends AbstractChainedBatch {
   }
 
   async _write (options) {
-    if (this.db.status === 'opening') {
-      return this.db.deferAsync(() => this._write(options))
-    } else if (this.db.status === 'open') {
-      if (this[kEncoded].length === 0) return
-
-      // Need to call the private rather than public method, to prevent
-      // recursion, double prefixing, double encoding and double hooks.
-      return this.db._batch(this[kEncoded], options)
-    } else {
-      throw new ModuleError('Batch is not open: cannot call write() after write() or close()', {
-        code: 'LEVEL_BATCH_NOT_OPEN'
-      })
-    }
+    // Need to call the private rather than public method, to prevent
+    // recursion, double prefixing, double encoding and double hooks.
+    return this.db._batch(this[kEncoded], options)
   }
 }
 

test/chained-batch-test.js

@@ -186,6 +186,52 @@ exports.batch = function (test, testCommon) {
     ])
   })
 
+  test('chained batch requires database to be open', async function (t) {
+    t.plan(5)
+
+    const db1 = testCommon.factory()
+    const db2 = testCommon.factory()
+
+    try {
+      db1.batch()
+    } catch (err) {
+      t.is(err.code, 'LEVEL_DATABASE_NOT_OPEN')
+    }
+
+    await db2.open()
+    const batch = db2.batch()
+    await db2.close()
+
+    try {
+      batch.put('beep', 'boop')
+    } catch (err) {
+      t.is(err.code, 'LEVEL_BATCH_NOT_OPEN')
+    }
+
+    try {
+      batch.del('456')
+    } catch (err) {
+      t.is(err.code, 'LEVEL_BATCH_NOT_OPEN')
+    }
+
+    try {
+      batch.clear()
+    } catch (err) {
+      t.is(err.code, 'LEVEL_BATCH_NOT_OPEN')
+    }
+
+    try {
+      await batch.write()
+    } catch (err) {
+      t.is(err.code, 'LEVEL_BATCH_NOT_OPEN')
+    }
+
+    // Should be a noop (already closed)
+    await batch.close()
+
+    return Promise.all([db1.close(), db2.close()])
+  })
+
   // NOTE: adapted from levelup
   test('chained batch with per-operation encoding options', async function (t) {
     t.plan(2)

test/deferred-open-test.js

@@ -37,27 +37,6 @@ exports.all = function (test, testCommon) {
     return db.close()
   })
 
-  // NOTE: copied from levelup
-  test('deferred open(): chained batch() on new database', async function (t) {
-    // Create database, opens in next tick
-    const db = testCommon.factory()
-    const entries = 3
-    const batch = db.batch()
-
-    // Add entries with batch(), these should be deferred until the database is actually open
-    for (let i = 1; i <= entries; i++) {
-      batch.put('k' + i, 'v' + i)
-    }
-
-    t.is(db.status, 'opening')
-    t.is(batch.length, entries)
-
-    await batch.write()
-    await verifyValues(t, db, entries)
-
-    return db.close()
-  })
-
   // NOTE: copied from levelup
   test('deferred open(): value of deferred operation is not stringified', async function (t) {
     const db = testCommon.factory({ valueEncoding: 'json' })

test/events/write.js

@@ -2,7 +2,12 @@
 
 module.exports = function (test, testCommon) {
   for (const deferred of [false, true]) {
-    for (const method of ['batch', 'chained batch', 'singular']) {
+    // Chained batch does not support deferred open
+    const batchMethods = deferred ? ['batch'] : ['batch', 'chained batch']
+    const allMethods = batchMethods.concat(['singular'])
+
+    for (const method of allMethods) {
+      // db.put() and db.del() do not support the sublevel option
       for (const withSublevel of (method === 'singular' ? [false] : [false, true])) {
         test(`db emits write event for ${method} put operation (deferred: ${deferred}, sublevel: ${withSublevel})`, async function (t) {
           t.plan(1)
@@ -100,7 +105,7 @@ module.exports = function (test, testCommon) {
       }
     }
 
-    for (const method of ['batch', 'chained batch']) {
+    for (const method of batchMethods) {
       test(`db emits write event for multiple ${method} operations (deferred: ${deferred})`, async function (t) {
         t.plan(1)
 
@@ -124,7 +129,7 @@ module.exports = function (test, testCommon) {
       })
     }
 
-    for (const method of ['batch', 'chained batch', 'singular']) {
+    for (const method of allMethods) {
       test(`db emits write event for ${method} operation in favor of deprecated events (deferred: ${deferred})`, async function (t) {
         t.plan(5)
 

test/self.js

@@ -478,12 +478,12 @@ test('test chained batch() (custom _chainedBatch) extensibility', async function
   t.is(spy.getCall(1).thisValue, test, '`this` on _chainedBatch() was correct')
 })
 
-test('test AbstractChainedBatch extensibility', function (t) {
+test('test AbstractChainedBatch extensibility', async function (t) {
   const Batch = implement(AbstractChainedBatch)
   const db = testCommon.factory()
+  await db.open()
   const test = new Batch(db)
   t.ok(test.db === db, 'instance has db reference')
-  t.end()
 })
 
 test('test AbstractChainedBatch expects a db', function (t) {
@@ -868,7 +868,6 @@ require('./self/abstract-iterator-test')
 require('./self/iterator-test')
 require('./self/deferred-iterator-test')
 require('./self/deferred-operations-test')
-require('./self/deferred-chained-batch-test')
 require('./self/async-iterator-test')
 require('./self/encoding-test')
 require('./self/sublevel-test')

test/self/deferred-operations-test.js

@@ -75,10 +75,10 @@ test('deferred operations are called in order', function (t) {
     { type: 'put', key: '041', value: 'b' }
   ])
   const it = db.iterator()
-  db.batch()
-    .put('050', 'c')
-    .put('051', 'd')
-    .write()
+  db.batch([
+    { type: 'put', key: '050', value: 'c' },
+    { type: 'put', key: '051', value: 'd' }
+  ])
   it.next()
   db.clear({ gt: '060' })