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

add tapError #1220

Merged
merged 7 commits into from Mar 3, 2017
Merged

add tapError #1220

merged 7 commits into from Mar 3, 2017

Conversation

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Aug 31, 2016

Adds Promise.prototype.tapError to bluebird.

The tests for the predicate version aren't passing yet (not sure why) and the doc is still missing, will hopefully follow up with those before I run out of momentum.

benjamingr added 2 commits Aug 31, 2016
@joepie91
Copy link

@joepie91 joepie91 commented Aug 31, 2016

Hmm. Is this meant to be equivalent to .error or to .catch?

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Aug 31, 2016

There should also be cancellation tests, see how .tap is tested in test/cancel.js

@@ -0,0 +1,128 @@
---
layout: api
id: tap

This comment has been minimized.

@petkaantonov

petkaantonov Aug 31, 2016
Owner

tapError

@@ -3,6 +3,8 @@ module.exports = function(Promise, tryConvertToPromise) {
var util = require("./util");
var CancellationError = Promise.CancellationError;
var errorObj = util.errorObj;
var NEXT_FILTER = {};

This comment has been minimized.

@petkaantonov

petkaantonov Aug 31, 2016
Owner

This needs to be the specific sentinel value that is declared in promise.js

This comment has been minimized.

@benjamingr

benjamingr Aug 31, 2016
Author Collaborator

Why?

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

Because otherwise it's just another object that's not special. This makes it special, and this NEXT_FILTER object will fail the check, because it's not the same.

@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Aug 31, 2016

Hmm. Is this meant to be equivalent to .error or to .catch?

To .catch (both the catch all variant and the filtered one.

@joepie91
Copy link

@joepie91 joepie91 commented Aug 31, 2016

To .catch (both the catch all variant and the filtered one.

Hmm, shouldn't we name it .tapCatch then? Otherwise, I can see this causing some confusion for users, thinking that it's equivalent to .error.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 1, 2016

Yeah tapCatch is probably a better name for this

@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Sep 1, 2016

  • Renaming to catch.
  • Fixing the doc issues.
  • I don't see tap in the cancellation tests.
  • Using the same NEXT_FILTER.

Predicates still don't work, PTAL.

return this._passThrough(handler,
TAP_TYPE,
undefined,
catchFilter(predicate, handler, this));

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

it should be catchFilter(predicate, finallyHandler, this)

This comment has been minimized.

@benjamingr

benjamingr Sep 1, 2016
Author Collaborator

wow, I spent like 20 minutes thinking what I'm not understanding in catchFilter and it ended up being a mistype -_-

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

also on top of modifying the finallyHandler, it should probably be:

       return this._passThrough(catchFilter(predicate, handler, this),
                                 TAP_TYPE,
                                 undefined,
                                 finallyHandler);

This comment has been minimized.

@benjamingr

benjamingr Sep 1, 2016
Author Collaborator

That doesn't fix it either.


[← Back To API Reference](/docs/api-reference.html)
<div class="api-code-section"><markdown>
##.tapError

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

s/tapError/tapCatch/g

undefined,
finallyHandler);
} else {
var predicate = arguments[0];

This comment has been minimized.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 1, 2016

I don't see tap in the cancellation tests.

never mind then, remembered wrong

@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Sep 1, 2016

@petkaantonov do we really need to support the multi parameter syntax? I'm fine with just supporting .tapCatch(e => {}) and .tapCatch(type|predicate, e => {})


describe("tap", function () {
specify("passes through rejection reason", function() {
return Promise.reject("test").tapError(function() {

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

Use new Error and assign it to variable, because this will cause a warning


specify("passes through reason after returned promise is fulfilled", function() {
var async = false;
return Promise.reject("test").tapError(function() {

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

Use new Error and assign it to variable, because this will cause a warning


specify("passes immediate rejection", function() {
var err = new Error();
return Promise.reject("test").tapError(function() {

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

Use new Error because this will cause a warning


specify("passes eventual rejection", function() {
var err = new Error();
return Promise.reject("test").tapError(function() {

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

Use new Error , because this will cause a warning

});

specify("passes reason", function() {
return Promise.reject(123).tapError(function(a) {

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

Use new Error and assign it to variable, because this will cause a warning

This comment has been minimized.

@benjamingr

benjamingr Sep 1, 2016
Author Collaborator

I got the point the first time :D

return Promise.reject(new TypeError).tapError(ReferneceError, function(a) {
called = true;
}).then(assert.fail, function() {
assert(called === false);

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

should also assert here that the error passed to this handler is the ReferenceError

specify("Works with predicates", function() {
var called = false;
return Promise.reject(new TypeError).tapError(TypeError, function(a) {
called = true;

This comment has been minimized.

@petkaantonov

petkaantonov Sep 1, 2016
Owner

also assert that a is the TypeError

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 1, 2016

No but the predicate should still be type checked at this point, otherwise confusing error will happen at some later point

@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Sep 1, 2016

Addressed the feedback but predicate tests still don't pass.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 1, 2016

The finallyHandler needs to be modified to:

if (ret === NEXT_FILTER) {
    return ret;
} else if (ret !== undefined) {
    ...
@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Sep 1, 2016

Still no dice

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 1, 2016

Run it individually to see the reason for failure node tools/test --run=tapCatch, for instance you have typed RefernceError instead of ReferenceError in one of the predicate tests

@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Sep 1, 2016

I did, that's not why it's failing. I see an assertionError, I don't really understand catchFilter very well - any chance you could take a look tonight before you release and see what I did wrong?

@benjamingr
Copy link
Collaborator Author

@benjamingr benjamingr commented Sep 9, 2016

@@ -53,7 +53,7 @@ redirect_from: "/docs/api/index.html"
- [Promise.using](api/promise.using.html)
- [.disposer](api/disposer.html)
- [Promisification](api/promisification.html)
- [Promise.promisify](api/promise.promisify.html)
- [Promise.promisify](api/promsie.promisify.html)

This comment has been minimized.

} else {
var predicate = arguments[0];
var handler = arguments[1];
return this._passThrough(catchFilter(predicate, handler, this),

This comment has been minimized.

@Retsam

Retsam Feb 27, 2017
Contributor

It looks like catchFilter is expecting an array of predicates, not a single predicate; this is why the "Works with predicates" test is failing.

called = true;
}).then(assert.fail, function(err) {
assert(called === false);
assert(err instanceof ReferenceError);

This comment has been minimized.

@Retsam

Retsam Feb 27, 2017
Contributor

I'm pretty sure this should be err instanceof TypeError.

@Retsam
Copy link
Contributor

@Retsam Retsam commented Feb 27, 2017

I'd love to see this feature go in; I've opened #1351 as an extension of this PR with my fixes for most of the above issues. (Still need to add type checking for predicates)

* Fix typo: ReferneceError -> ReferenceError

* Fix typo: promsie -> promise

* s/tapError/tapCatch/g

* Pass an array of predicates to catchFilter in tapCatch

* Fix tapCatch tests

* Add support for multiple predicates and predicate type checking
@petkaantonov petkaantonov merged commit 053d02e into master Mar 3, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@petkaantonov petkaantonov deleted the add-tapErrr branch Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants