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

onFulfilled not a function/onRejected not a function #25

Closed
ForbesLindesay opened this issue Nov 7, 2012 · 56 comments
Closed

onFulfilled not a function/onRejected not a function #25

ForbesLindesay opened this issue Nov 7, 2012 · 56 comments

Comments

@ForbesLindesay
Copy link
Member

It seems for points 3 and 5 it seems that it would be reasonable to only ignore 'falsy' values and to throw on other non-functions?

@domenic
Copy link
Member

domenic commented Nov 7, 2012

Having then have synchronous behavior, like throwing, is a bad idea.

@ForbesLindesay
Copy link
Member Author

Yes, in general, but it would be throwing in a miss-use case, which is different to throwing in a more dynamic way. I don't think it's ideal to ignore things like:

getDataFromServer().then(5, true);

I think the promise library should be allowed to throw an exception in that case. It would be wrong to throw a syncronous error with:

getDataFromServer().then(function () {
 throw new Error();
});

because its impossible to trap, and likely to occur at runtime, rather than in debug/development.

@briancavalier
Copy link
Member

Hmmm, right, failing early and loudly during dev is a good thing. The spec currently says they must be ignored if they aren't functions. In when.js we do check to see if param == null || typeof param === 'function' in some places (not in then(), tho). So what about the possibility of saying onFulfilled and onRejected each "must be a function, null, or undefined", and allowing an implementation to throw if either isn't?

@domenic
Copy link
Member

domenic commented Nov 7, 2012

No, I really dislike this idea. I never want to see an error from then. Maybe as part of a non-compliant debug mode (similar to a contemplated debug mode where unhandled rejections are not stored but instead thrown immediately), but not in a normal situation.

More philosophically, argument type checking is not "the JS way," for better or for worse.

@briancavalier
Copy link
Member

I tend to prefer "help the developer" over "the JS way", when they don't happen to intersect. I do agree that a non-compliant "debug mode" can help here. However, if a library author decides to make then fail fast because he/she feels it helps developers, two questions: 1) what do you feel is bad about that, and 2) why do you feel we should prohibit it in the spec?

Or maybe coming at it from another angle: How would a developer track down a problem that occurs from a mistake where he/she passes in something that's not a function, if .then() silently ignores it?

@domenic
Copy link
Member

domenic commented Nov 7, 2012

I tend to prefer "help the developer" over "the JS way", when they don't happen to intersect.

Generally I agree, but:

However, if a library author decides to make then fail fast because he/she feels it helps developers, two questions: 1) what do you feel is bad about that, and 2) why do you feel we should prohibit it in the spec?

To answer both, I feel that we should specify that then has no synchronous behavior at all.

How would a developer track down a problem that occurs from a mistake where he/she passes in something that's not a function, if .then() silently ignores it?

The same way they track down passing a function to parseInt: notice that the results are not what they expect, and go from there. Not ideal, I agree, but that would be my preferred balance.


I'm willing to be overruled on this but would like some other parties to chime in. The phrasing I'd possibly be OK with is "3. If onFulfilled is falsy, it must be ignored" with a note at the bottom clarifying that the behavior for non-falsy non-functions is left unspecified, perhaps expanding on the reasons for and against silently ignoring.

@lsmith
Copy link
Contributor

lsmith commented Nov 7, 2012

I agree that then shouldn't error. The spec is simpler for ignoring non-function values, and I think simplicity makes it easier to implement and promote/spread.

Naturally vars will be passed into then(), but in the case of a promise API, passing anything other than a function, undefined, or null is a mistake. In the case of calling then(???, errback), null support is required IMO to avoid treating undefined as a value. Mirroring this in then(callback, null) should then be acceptable, and now we would have to test against three value types before throwing.

Introducing thrown errors means added complexity when sharing promises across systems (which may be inevitable, but having a spec on your side is good).

I'm usually all for making the dev's life easier, but in this case, I agree with @domenic that then should be reliably async.

@lsmith
Copy link
Contributor

lsmith commented Nov 7, 2012

I'm willing to be overruled on this but would like some other parties to chime in. The phrasing I'd possibly be OK with is "3. If onFulfilled is falsy, it must be ignored" with a note at the bottom clarifying that the behavior for non-falsy non-functions is left unspecified, perhaps expanding on the reasons for and against silently ignoring.

I considered this, too, but the purpose of A+ is to fill in the missing details of A. We're already leaving out progress, so I'd rather avoid leaving too many options open for implementation. Be definitive.

@briancavalier
Copy link
Member

@lsmith even though it's essentially a "compile time" error? i.e. in a typed language, a developer would almost always find this error before any code even executes. Just to be clear: I'm not suggesting that we require type checking in the spec, I'm just wondering if we should word the spec in such a way that it's not disallowed, such as @domenic's suggested phrasing.

@briancavalier
Copy link
Member

Introducing thrown errors means added complexity when sharing promises across systems

@lsmith can you explain in a bit more detail what you mean here?

@unscriptable
Copy link
Member

@domenic: If you are promoting "the JS way" -- and "the JS way" means to fail obscurely -- I don't know what to say next. :)

I understand why a native function, like parseInt(func), may not cause a run-time error until some time in the future (or never at all). However, others that can't reasonably be expected to coerce to something reasonable, don't:

> [1,2,3].forEach("hello");
TypeError: hello is not a function

@briancavalier
Copy link
Member

Right, and the Array iterators are actually spec'd that way. They must throw. Again, I'm not advocating that we require it, just that it seems harsh to prohibit it. It's so clearly a programmer error that it just feels wrong to force an implementation to eat it silently.

@lsmith
Copy link
Contributor

lsmith commented Nov 7, 2012

even though it's essentially a "compile time" error?

Errors will occur during dev or in production. In either case, the crux is the assignment timing and values given to varA and varB in promise.then(varA, varB). Either they're scoped vars or object methods.

  • Having a var destined to pass to then() contain a variety of value types is smelly.
  • Having a var whose value will be assigned in a separate operation creates a race that might show odd behavior in dev or testing, but races make their way to production, and errors in production are bad.
  • Object methods scoped to that object using bind or equivalent, which would throw an error at compile time (race conditions noted in the prior point notwithstanding)
  • Object methods referenced from an object that exists, but without binding and the method is not set? See above points.

I think the nature of the API is such that args will be passed with certainty, and the likelihood of args being passed with less certainty won't gain significantly from thrown errors in a way that doesn't jeopardize errors being thrown in production.

Array iterators are actually spec'd that way. They must throw.

This is a more compelling point. If the goal of the spec is to model an API that could be adopted into native ES, it makes sense to follow that precedence. I'm now less firmly in the "no throw" camp, but still leaning that way.

can you explain in a bit more detail what you mean here?

Glad you asked. In thinking on it more, I can't come up with a useful example case. There may be one/some, but none are coming to mind now.

@lsmith
Copy link
Contributor

lsmith commented Nov 7, 2012

@unscriptable @briancavalier comparing to forEach() is apples and oranges because onFulfilled and onRejected are both optional. The question isn't whether to throw for non-functions, it's whether to throw for non-function-and-non-X.

@lsmith
Copy link
Contributor

lsmith commented Nov 7, 2012

An example of optional function treatment is in JSON.parse(str[, reviver]) and JSON.stringify(o[, replacer[, space]]) which ignores non-isCallables. http://es5.github.com/#x15.12.2

Others?

@briancavalier
Copy link
Member

I think one difference here is that, even though they are both optional, a developer will almost always supply at least one of onFulfilled or onRejected. IOW, promise.then() (zero args) is something you'll probably never find yourself doing, even though it's legal. That makes it slightly different, at least in my mind, from something like JSON.parse. It's almost as if then requires at least one of its arguments to be provided.

it's whether to throw for non-function-and-non-X

Yes, I'm saying that my current favorite option here is not to prohibit an implementation from sync throwing for a truthy, non-function value.

Interestingly, I just re-read Promises/A, and it does dictate that any non-function arg must be ignored:

All arguments are optional and non-function values are ignored

And to be fair, I can't remember hearing of anyone getting bitting by this mistake using when.js, so it's possible that it's just not the kind of thing that trips people up. Or, as @lsmith put it:

I think the nature of the API is such that args will be passed with certainty

Hmmm ... more thinking required ...

@ForbesLindesay
Copy link
Member Author

I'm curios as to what other implementations do about this in the wild. What does Q do for example? I'm not saying we should necessarily force libraries to do type checking, I'm saying that we probably should leave that behaviour unspecified in the case of 'truthy' non-functions.

As I see it there are three main things that need to be achieved by promises-aplus:

  1. provide interoperability to promises. All libraries that are built to handle promises must be able to rely on promises matching this spec.
  2. provide a sane and sensible API to developers. This is the reasoning behind things like never calling handlers in the same turn of the event loop as the call to then.
  3. provide a simple enough proposal that all (at least all popular) promises implementations will meet the spec.

I think it's fair to say that it would be very bad practice for any promises library to call then with a 'truthy' non-promise (see my earlier example). So I don't think 1 is an issue here at all.

Point 2 is then up for debate. The values sent to then should always be fairly consistent, it would be very bizzar to send user input straight to the then method:

//please don't do this
getDataFromServer().then(eval(document.getElementById('inputID').value));

so if it's going to fail, it's going to fail at development time and it's going to fail consistently. I think this is the most likely expected behaviour from a users point of view. If I pass the array ['foo', 'bar', 'baz'] as the completion handler, there's a good bet I meant to do something else. Ideally I'd like it to throw an error straight away saying I gave it the wrong argument, failing that I'd like it to at least throw an error (Even if it's a bit obscure I can see where it came from). Failing that I'd like it to reject the promise with a justification of the incorrect arg, that way I can still see where it came from. The one thing I really don't want it to do is swallow it, making the error effectively un-observable.

In terms of point 3, I think that we should consider what current implementations do. If most ignore all non-functions, we should probably go ahead and require them to do so, making all promises more consistent. If we don't though, we should probably take it that ignoring just falsy values is the more natural way to go, so we should probably go with that.

@ForbesLindesay
Copy link
Member Author

Interestingly, jQuery seems to swallow strings for optional function arguments in jQuery.ajax. I suspect this is just an issue people don't often notice. Args tend to be passed with certainty and it tends to be obvious what sort of arg should be passed.

@domenic
Copy link
Member

domenic commented Nov 9, 2012

Ugh, GitHub closes if you push to a non-master branch too. This is still open.

@domenic
Copy link
Member

domenic commented Nov 15, 2012

In my opinion, under the "don't leave things unspecified" school of thought, we have two options:

  1. Keep the current language and demand that non-function arguments are ignored.
  2. Throw on non-undefined, non-functions.

Let's take a quick straw poll:

  • Do you agree these are the two options?
  • Which of these do you prefer?

My answers of course being "yes" and "1," but I could live with 2 if @kriskowal can.

I'd also be curious if TC39 has an opinion; perhaps if we can't reach a consensus we could ask es-discuss.

@ForbesLindesay
Copy link
Member Author

My vote: no and 2.

I'd like to propose:

  1. Leave non-undefined, non-functions unspecified

I think we should see if any de-facto standards emerge, and then we can specify this in promises/A++

@briancavalier
Copy link
Member

my straw: no and 2

@juandopazo
Copy link
Contributor

Yes and (1), though I'm not 100% convinced.

If it were to be (2), then I'd add non-null.

@domenic
Copy link
Member

domenic commented Nov 15, 2012

I was just about to jump in and counterargue with @ForbesLindesay, but I like the non-argumentative stating of opinions the straw poll has produced.

Let's say we wait until 17:30 UTC (i.e. 1:10 from now) to let things settle, both here and in our respective heads, before wading back into things :)

@unscriptable
Copy link
Member

yes* and 2

*I could live with this since it does feel very ES-ish like Domenic points out, but I could also be swayed to allow certain other falsey values in as well.

@kriskowal
Copy link
Member

I don’t mind mandating that then throws if an argument is not null and not undefined and not a function.

@briancavalier
Copy link
Member

@kriskowal I understand, and we're in a similar situation in when.js. There is usage in the wild, and it's shown in our docs. There's no doubt it would be a pretty big deprecation and require a major version bump to 2.0, but it's not impossible. We could do it.

Q isn't yet at 1.0, so it seems like if there were ever a time to do this, it would be now.

It would be good to hear from @wycats and @lsmith on this, as well.

@ForbesLindesay
Copy link
Member Author

It's not like we'd need to start throwing immediately, we could have code like:

if (typeof callback != 'function' && tyepof callback != 'undefined' &&
    typeof console == 'object' && typeof console.warn == 'function') {
  console.warn('Passing ' + JSON.stringify(callback) + ' as a callback is deprecated, pass undefined instead.');
}

I still really don't think this is something that is needed to allow promises to interoperate properly. I think we should be keeping this open for the future. If we don't spec this behaviour yet, we could spec it in the future, and not break backwards compatibility, because functions like Q.all would continue to work regardless of the behaviour we specify for when callback is not undefined and not callable. If we specify that such args will be ignored, they must be ignored for all time.

I think we're beginning to get to the point where the discussion is going round in circles though, and the issue needs resolving. I think either we should have a vote (perhaps just of the n-most popular promise library's maintainers) or try and get someone with spec writing experience (from TC39??) to give some advice.

@ForbesLindesay
Copy link
Member Author

Perhaps we should also try and involve the people on esdiscuss

@briancavalier
Copy link
Member

I'd say that we've made progress, and we've narrowed to a very specific question:

Should we allow null to indicate default behavior in addition to undefined, or should we strictly limit default behavior to undefined?

IMHO, it's a very good thing that our process so far has been light weight. If we could get focused advice and additional perspective on this particular issue, I think it would be helpful.

@awwx
Copy link

awwx commented Nov 16, 2012

As a newbie to promises, I would prefer that an implementation be allowed to not ignore non-functions:

  • In Javascript it's pretty obvious that I forgot the function () {...}; but, while I usually enjoy the brevity of CoffeeScript, I find that I often accidentally leave out a -> (which creates a function in CoffeeScript) when calling then. While a simplistic mistake, I would benefit from an implementation being allowed to quickly notify me that I did this dumb thing.
  • Requiring non-functions to be ignored does not strike me as being useful. Any correctly working program under the current spec that passes in a non-function will work the same if it passed in undefined instead, and so the requirement adds no value.

I argue against having a "non-compliant debug mode" because that means that programs written correctly against the spec will fail in the debug mode. Consider this scenario:

  • I pass my promise to a library which uses .then("do nothing", callback) as a permitted way to indicate that they are deliberately leaving out the first callback.
  • my code uses the library, but I'm unable to use the "non-compliant debug mode" because it causes the library to break.

If an implementation is allowed to respond to non-functions, then I also have a preference that it be allowed to have then throw synchronously when passed an invalid callback:

  • I agree that then used correctly should never throw. However, promises are essentially a DSL for constructing asynchronous code. Passing in a non-function is similar to misspelling then; it is a compile-time or "construction-time" error, not a run-time error. Throwing the error synchronously is "failing fast": I see the error immediately, before the code runs, instead of later or not at all. Thus it is useful and appropriate for the error to be raised synchronously, just as other construction-time errors are raised synchronously.
  • I prefer a synchronous error instead of having the implementation generate a rejected promise because when I write code using promises, the reason values of rejected promises are reasons that my code generates (either by throwing an error inside of a callback or by rejecting a promise). I can thus write code that expects particular rejected promise reasons. If the implementation adds its own rejection reasons, than I have to write code to handle these "other" reasons, and I have to include that code in every onRejected callback I write.

On the subject of null and undefined, I prefer that null be allowed:

  • Conceptually, undefined means "the value of a variable that has not been assigned a value" (ECMA-262 Edition 5.1 section 4.3.9) and null means "the intentional absence of any object value" (section 4.3.11). Since I'm intentionally leaving out e.g. the onFulfilled callback, using null is appropriate.

I would in fact suggest that null be the canonical value for not specifying a callback, and that also allowing undefined is a convenience.

@novemberborn
Copy link
Contributor

FYI Dojo's implementation invokes the callbacks if they're truthy: it assumes they're functions. Any error in invocation (including invoking a truthy non-function callback) causes the returned promise to be rejected.

@lsmith
Copy link
Contributor

lsmith commented Nov 19, 2012

Sorry for my absence, I was out of town.

For @domenic's poll, yes and (1), but barring that I could accept either (2)+null or (2)+truthy, for the reasons @kriskowal and @novemberborn mention. I noted my preference for null over undefined in my first comment. My first choice is definitely (1), though.

@awwx

I would in fact suggest that null be the canonical value for not specifying a callback, and that also allowing undefined is a convenience.

I would agree that null be the recommended value for not passing onFulfilled specifically. Broadening that to "for not specifying a callback" could be interpreted as recommending promise.then(callback, null), which confuses the term "optional". I'm guessing this is what you meant, but just wanted to clarify.

For another implementation reference, Crockford's Vows (https://github.com/douglascrockford/monad/blob/master/vow.js) ignores non-functions.

@domenic
Copy link
Member

domenic commented Nov 19, 2012

I find it very strange people are trying hard to standardize on something that not a single current implementation does. Are you all that unhappy with the current de-facto standard of ignoring non-functions?

@ForbesLindesay
Copy link
Member Author

I'm not that fussed given the options of 'throw on non-functions' and 'ignore non-functions'. I just think it's a shame that we're prohibiting clever things as extensions:

A short-cut for returning constant values (except undefined and null);

/**
 * @return {Promise<bool>}
 */
function saveToServer() {
  return post('http://foo.com', data).then(true);
}

A version that parses strings that look like ES6 style => functions.

function getJSON(url) {
  return get(url).then('(data) => JSON.parse(data)');
}

I accept that this doesn't seem to match that many people here's opinions though, so I'm willing to back down and accept whatever consensus can be reached on this.

@briancavalier
Copy link
Member

@ForbesLindesay I totally agree that's useful functionality, but imho, it's the kind of thing that should be provided as a separate method, not as an extension to .then().

@awwx
Copy link

awwx commented Nov 19, 2012

@domenic

I find it very strange people are trying hard to standardize on something that not a single current implementation does.

Is a concern that without real-world experience with the proposal, it may be risky to mandate it in the specification?

@lsmith

Broadening that to "for not specifying a callback" could be interpreted as recommending promise.then(callback, null), which confuses the term "optional". I'm guessing this is what you meant, but just wanted to clarify.

Oh no, I didn't mean to disparage using optional arguments.

@kriskowal
Copy link
Member

My position: Q will always accept null, undefined, and functions as arguments to then. I might make it throw an error if you provide something else, or I might leave it as-is, in much the same way that I might make it throw an error if you provide seven arguments instead of one, two (or three).

@lsmith
Copy link
Contributor

lsmith commented Nov 20, 2012

Returning to the origin of A+,

This proposal attempts to clarify the behavioral clauses of the Promises/A proposal, to extend it to cover de facto behaviors, and to omit parts that have proven to be underspecified or problematic.

Without risking the language in that sentence, ignoring non-functions is de facto, and doesn't seem to have proven problematic. Adding a throw in the spec means existing implementations will need to be updated or ignore the spec. I want broad compliance, which hinges on implementers being willing to accept the costs of compliance. Zero cost is ideal. This would introduce a cost to all implementations.

@briancavalier
Copy link
Member

I fully respect the desire to go with de facto behaviors. It certainly provides the least amount of friction for existing implementations. As I said over in #32, if the other implementors here agree it is the right way to go, I will reluctantly go along.

However, I don't think de facto behavior can be the only concern. The fact is the debugging promises is painful, especially for those new to promises to whom something like this is more likely to happen, and I see this as a chance to help that situation, even if only slightly, and improve upon Promises/A. IMHO, we need to consider both developers and implementors.

All of that said, I think it's very important for this spec to keep moving and reach 1.0 sooner rather than later. Based on the most recent comments about null, it seems there there are only two realistic options for reaching consensus:

  1. Silently ignore all non-functions (i.e. then must not reject the returned promise or throw for non-functions).
  2. Allow only null, undefined, or function. Must throw otherwise.

So, let's have another straw pull. 1 or 2.

@briancavalier
Copy link
Member

2

@domenic
Copy link
Member

domenic commented Nov 20, 2012

1

@kriskowal
Copy link
Member

(3) Must accept null, undefined, or function. May throw otherwise.

@lsmith
Copy link
Contributor

lsmith commented Nov 20, 2012

1

@kriskowal
Copy link
Member

1 if 3 does not fly.

@ForbesLindesay
Copy link
Member Author

3, and failing that 1

@briancavalier
Copy link
Member

Ok, it seems that 2 does not have enough support. The most viable options seem to be:

  1. Silently ignore all non-functions (i.e. then must not reject the returned promise or throw for non-functions).
  2. Must accept null, undefined, or function. May throw otherwise.

Of those, 1 seems to be the most acceptable since the 2 votes for 3 had contingency clauses :)

The current spec language is, afaict, sufficient for 1. If you absolutely, positively cannot live with 1, reopen this issue.

@awwx
Copy link

awwx commented Nov 20, 2012

But @briancavalier, what is your vote? The tally currently stands:

1: domenic, lsmith
3: kriskowal, ForbesLindesay

2 lost in the first round, but you're still entitled your vote in the run-off election :)

As a member of the public, I would wish to hear more arguments for why to chose 1 over 3. I've heard the argument that then should never throw, but not why this is problematic for construction-time errors. Aside from that, it remains unclear to me why it's more important to nail down the behavior of non-functions than it is to give implementations the freedom to choose to do something useful or helpful -- if they would like to.

@briancavalier
Copy link
Member

Thanks @awwx :) To be completely honest, though, 1 and 3 are a toss up for me. That sounds like a cop-out, but if "must throw" is not an option, then our plan for when.js will be to provide the best possible developer feedback via when/debug while staying reasonably within the boundaries of the spec.

If @kriskowal or @ForbesLindesay do feel strongly enough about 3, then they should certainly reopen this. I'm ready to move on, tho.

@unscriptable
Copy link
Member

I think the only sane option is 2. It is the most javascript-like and is the most friendly to devs. Not gonna drag this out, though, if everybody wants to be insane. :)

Fwiw, I just found another optional function parameter that throws, although this is in an ES7 proposal: http://wiki.ecmascript.org/doku.php?id=strawman:data_parallelism#scatter (see conflictFunction)

Like @domenic suggested earlier, it only accepts undefined or a function, throws on everything else.

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 a pull request may close this issue.

9 participants