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

Making Q.set and Q.delete chainable #705

Closed
wants to merge 1 commit into from
Closed

Making Q.set and Q.delete chainable #705

wants to merge 1 commit into from

Conversation

thefourtheye
Copy link

When I was answering this question in Stackoverflow, I realized that we cannot do something like this

Q({ foo: "bar" }).set("foo", "baz").then(console.log);

Because, set doesn't resolve with the modified object, so console.log would print undefined. Also, this limits the users from doing, something like this

Q({ foo: "bar" }).set("foo", "baz").get("foo").then(console.log);

The intention of this PR is to make set and delete to resolve with the modified object, so that they can be chained in the promise flow.

@@ -557,11 +557,21 @@ describe("promises for objects", function () {
return Q(object)
.set("a", 1)
.then(function (result) {
expect(result).toBe(undefined);
expect(result).not.toBe(undefined);
expect(object).toBe(result);
Copy link

Choose a reason for hiding this comment

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

expect(result).toBe(object) sounds more natural

Copy link
Author

Choose a reason for hiding this comment

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

@bergus Thanks for reviewing :-) I changed it like that.

@bergus
Copy link

bergus commented Jun 20, 2015

+1. Nobody needs a promise that resolves to undefined.

@kriskowal
Copy link
Owner

Deep in the annals of Q, you will find that this backward-incompatible change reverts a backward incompatible change that was made between 0.8 and 0.9. I decided that it would be suboptimal for Q-Connection to be hauling the serialized return value of these methods across the wire. @domenic also expressed distaste for chaining style methods, and I concur.

With the respect due for a proposal to make Q work the way I originally wrote it, this will not land in the v1 branch (because it is backward-incompatible) and probably won’t land in the v2 branch (because it would lead to waste on the wire).

@kriskowal kriskowal closed this Jun 20, 2015
@thefourtheye thefourtheye deleted the fixing-get-and-delete branch June 20, 2015 20:49
@benjamingr
Copy link
Collaborator

@kriskowal just pointing out - the "across the wire" thing can be handled by a second method like:

Q({ foo: "bar" }).set("foo", "baz").ignoreReturnValue()...

Not that it matters as you're super busy anyway on other stuff :)

@kriskowal
Copy link
Owner

@benjamingr That would not work. The .set() method sends a message, which will arrive before the "ignore" message.

@benjamingr
Copy link
Collaborator

@kriskowal not if ignoreReturnValue is called synchronously. The messages set sends are presumably batched anyway so presumably you don't dispatch them "right away" but after a tick - so plenty of time to augment whatever message sent to not return anything from the server but an ack.

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

Successfully merging this pull request may close these issues.

None yet

4 participants