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

Encourage warning messages for onFulfilled or onRejected being promises #134

Closed
wants to merge 1 commit into from

Conversation

ForbesLindesay
Copy link
Member

This is probably the most common error I see from beginners using promises on stack overflow (e.g. http://stackoverflow.com/questions/17517593/more-complex-deferred-promise-usage-and-chains-just-arent-clicking-would-love/17525172). They call the function, rather than passing the function to .then. Since I can't see why anyone would intentionally pass a promise to a .then handler and these sorts of functions almost always return promises, I think this could be a big win for helping people get started faster.

P.S. the PR is just there as a discussion point, I'm happy to re-word/move it as appropriate.

@bergus
Copy link

bergus commented Jul 16, 2013

Actually I think we should recommend this for all truthy, non-function values. There are many callbacks that produce non-promise values, and they should trigger a warning as well.

@domenic
Copy link
Member

domenic commented Jul 16, 2013

I don't think the spec should be in the business of handing out warnings. I admit the circular thenable chain scenario is a bit of a slipper slope, but I'd rather leave this up to implementers.

If anything I'd go in the direction of creating a best-practices document for implementers, with suggestions like "warn on non-undefined non-functions passed to then," or "provide a resolve method but not a reject one," and then we could move the circular thenable chain error into that document.

@ForbesLindesay
Copy link
Member Author

My feeling at the moment is that there's an implicit "and do nothing else" clause effectively associated with every point of the spec. Over-zealous warnings could lead to a very broken promise library. I think there should at least be some advice on the sorts of things it is and isn't ok to produce a warning for.

I like the idea of a separate document for these sorts of things, but I think it belongs in this repository so that it at least gets the promises/A+ rubber stamp :)

@briancavalier
Copy link
Member

I'd be ok with putting this kind of thing in a separate doc.

@domenic
Copy link
Member

domenic commented Aug 20, 2013

Let's close this and open a more general best-practices-for-implementers doc issue.

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.

4 participants