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

Proper destroy #34

Merged
merged 2 commits into from
Jun 24, 2018
Merged

Proper destroy #34

merged 2 commits into from
Jun 24, 2018

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Jun 24, 2018

  • Fixed: end the iterator on a next() error (closes Call destroy() on next() error #33)
  • Added: emit close after error (mimicking node 10) (before this PR, error was the last event)
  • Added: support .destroy(err) (node 8)
  • Added: support .destroy(err, callback) (node 8).

@vweevers vweevers added this to Backlog in Level (old board) via automation Jun 24, 2018
@vweevers vweevers moved this from Backlog to Review in Level (old board) Jun 24, 2018
this._iterator.end(function (err2) {
callback(err || err2)

// TODO when the next readable-stream (mirroring node v10) is out:
Copy link
Member

Choose a reason for hiding this comment

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

Cool. Didn't know about this.

stream.on('close', onClose)
}

return order
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

test.js Outdated
})

iterator.next = function (cb) {
cb(new Error('next'))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe do process.nextTick(cb, new Error('next'))? (Assuming we only run tests in node that is)

@ralphtheninja ralphtheninja merged commit 9460d1b into master Jun 24, 2018
Level (old board) automation moved this from Review to Done Jun 24, 2018
@ralphtheninja ralphtheninja deleted the destroy branch June 24, 2018 22:07
@vweevers
Copy link
Member Author

@ralphtheninja wait, what about this?

@ralphtheninja
Copy link
Member

@ralphtheninja wait, what about this?

Sorry for being sloppy. I'll fix.

@vweevers
Copy link
Member Author

Np. I guess these could be considered fixes (because we're now api-compatible with streams):

Added: support .destroy(err) (node 8)
Added: support .destroy(err, callback) (node 8).

But I'm not sure about:

Added: emit close after error (mimicking node 10) (before this PR, error was the last event)

@ralphtheninja
Copy link
Member

ralphtheninja commented Jun 28, 2018

Geesh I'm messing things up today.

So, to be sure, I could release v2.0.3 which includes a revert of this change.

@vweevers
Copy link
Member Author

I don't really have energy left to think about this, but reverting sounds comforting ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Call destroy() on next() error
2 participants