Deprecate can.isDeferred. More accurate can.isPromise. #2247

Merged
merged 6 commits into from Apr 7, 2016

Conversation

Projects
None yet
4 participants
@nlundquist
Collaborator

nlundquist commented Feb 9, 2016

Resolves #1747, closes #2359

isPromise needs to explicitly reject can.List as it shouldn't be handled as a promise and the can.List promise plugin is likely to be removed in a coming release.

util/can.js
+ can.isPromise = function(obj){
+ return obj && (
+ (window.Promise && (obj instanceof Promise)) ||
+ (can.isFunction(obj.then) && can.isFunction(obj.catch || obj.fail) && !(obj instanceof can.List))

This comment has been minimized.

@matthewp

matthewp Feb 9, 2016

Contributor

I don't think checking for catch/fail is right as this is not a required to be a Promise. This is probably the way to do it: https://github.com/then/is-promise/blob/master/index.js

@matthewp

matthewp Feb 9, 2016

Contributor

I don't think checking for catch/fail is right as this is not a required to be a Promise. This is probably the way to do it: https://github.com/then/is-promise/blob/master/index.js

This comment has been minimized.

@nlundquist

nlundquist Feb 9, 2016

Collaborator

Fair enough. Looking at the diffferences to what you linked, you think I should just remove the isFunction(catch || fail)? Or should I also check if the argument is an object or function?

@nlundquist

nlundquist Feb 9, 2016

Collaborator

Fair enough. Looking at the diffferences to what you linked, you think I should just remove the isFunction(catch || fail)? Or should I also check if the argument is an object or function?

This comment has been minimized.

@matthewp

matthewp Feb 9, 2016

Contributor

Not sure, wondering what scenario it is a function. Should this work with jQuery's deferreds? If so I'd make sure that it does (think it should)

@matthewp

matthewp Feb 9, 2016

Contributor

Not sure, wondering what scenario it is a function. Should this work with jQuery's deferreds? If so I'd make sure that it does (think it should)

This comment has been minimized.

@nlundquist

nlundquist Feb 9, 2016

Collaborator

Yeah this works with jQuery deferreds.

@nlundquist

nlundquist Feb 9, 2016

Collaborator

Yeah this works with jQuery deferreds.

util/can.js
+ return can.isPromise.call(this, arguments);
+ };
+ can.isPromise = function(obj){
+ return obj && (

This comment has been minimized.

@matthewp

matthewp Feb 9, 2016

Contributor

Make this be !!obj so it returns false when provided undefined. This function should always return a boolean.

@matthewp

matthewp Feb 9, 2016

Contributor

Make this be !!obj so it returns false when provided undefined. This function should always return a boolean.

util/can.js
+
+ can.isDeferred = function() {
+ can.dev.warn('can.isDeferred: this function is deprecated and will be removed in a future release. can.isPromise replaces the functionality of can.isDeferred.');
+ return can.isPromise.call(this, arguments);

This comment has been minimized.

@matthewp

matthewp Feb 9, 2016

Contributor

This changes the behavior of isDeferred. If it's going to be deprecated it should have the same behavior until it is removed.

@matthewp

matthewp Feb 9, 2016

Contributor

This changes the behavior of isDeferred. If it's going to be deprecated it should have the same behavior until it is removed.

This comment has been minimized.

@nlundquist

nlundquist Feb 9, 2016

Collaborator

Totally right.

@nlundquist

nlundquist Feb 9, 2016

Collaborator

Totally right.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Apr 5, 2016

Contributor

@daffl should this go into major?

Contributor

matthewp commented Apr 5, 2016

@daffl should this go into major?

test/pluginified/2.0.5.test.js
@@ -316,7 +316,7 @@ var __m1 = (function () {
update: function () {
var deferred = this.scope.attr('deferreddata'),
scope = this.scope;
- if (can.isDeferred(deferred)) {
+ if (can.isPromise(deferred)) {

This comment has been minimized.

@justinbmeyer

justinbmeyer Apr 5, 2016

Contributor

no changes should be made in this file. This is our legacy test.

@justinbmeyer

justinbmeyer Apr 5, 2016

Contributor

no changes should be made in this file. This is our legacy test.

@daffl daffl modified the milestones: 2.3.23, 2.4.0 Apr 5, 2016

@daffl daffl merged commit 91a9ca8 into master Apr 7, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the 1747-better-promise-detection branch Apr 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment