Handle rejected promises from `router.param`

[jonchurch]

Closes #91

Checks if function passed to router.param returns a promise, catches rejected promise.

Ended up reusing the same approach from handling rejected promises from middleware.

Moved the isPromise function used for handling middleware promises into a new util folder to share the logic without duplicating it.

[dougwilson]

Annoyingly more often than we wish, folks end up "reaching in" to our package to require files because... reasons (IDK why). We should probably just drop in this module instead https://github.com/then/is-promise

[jonchurch]

You mean so no one can try and require the isPromise util we'd be exposing publicly?

[jonchurch]

Ughhh my auto linter ran on the readme, fix incoming

[dougwilson]

Yea. But also the bonus that ideally they have a test suite to know what is and is not a promise, unlike our coped-in one. A module like that (which has no dependencies) is an easy one to add, as we can depend on a specific version without risk of module hijacking.

[jonchurch]

Okay switched to using the is-promise package, squashed to cleanup, and then pinned is-promise to it's current version.

[dougwilson]

I'm not sure why Travis CI failed, as the log is not loading for me, but based on the version numbers, it is likely because the new testes are not contained in a promise test. See https://github.com/pillarjs/router/pull/60/commits/d2bce0c09d424ea89e77cef7743e12f411ed94f7 for an example. Note the describePromises helper and wrapping promise tests within it.

[dougwilson]

So I believe all the comments have been addressed now. Any objections to merging at this point?

[wesleytodd]

I wound't consider them blocking, but my comments on the docs are not fully addressed. So no opposition to merging, but I stand by my statements on the docs changes.

[dougwilson]

Apologies Wes, I missed those doc comments when looking over. Yes, the doc suggestions make sense to me to land in the PR. This is exactly why i wanted to ask before doing anything, to make sure nothing got missed in all the comments above :)

[jonchurch]

Been spaced these past couple weeks, dunno why I didn't commit the doc changes when they came in hehe. I committed the doc suggestions as is @wesleytodd.

I haven't done much testing around the stack trace changes suggested. I think it's definitely worth looking into. It pertains to the middleware rejection catching too though, so it's probably best addressed in another PR.

[dougwilson]

So the doc changes have been committed here, and I know there is still an ongoing thread around the stack traces. Is the stack trace discussion something to open a separate issue or PR about, or do we want to continue it here prior to landing? Mainly looking for confirmation from Wes on that :)

Edit: sorry for the close/open, was typing this on mobile.

[mastermatt]

This was brought up in the TC meeting yesterday.

Is the use of is-promise (or any Promise sniffer) being used because v2.0 is going to be maintaining Node support <0.12?

[dougwilson]

I believe the reason is that we are not requiring everyone re-write their handlers & middlewares as async function or to return a promise, so the code needs to determine if there was a returned value was was a promise or not. I looked through on promises API in the standards and there does not seem to be any built-in method to detect if something is a promise or not, is that right? The closest would be instanceof Promise, thought that would not work if users are returning user-land-based promises (like those from bluebird, et. al.).

[mastermatt]

My understanding is that the expectation when wanting to support promise and non-promise in the same flow is to put the variable into Promise.resolve. But this only works in Node 0.12+.

try {
    var ret = fn(req, res, paramCallback, paramVal, key.name)
    Promise.resolve(ret).then(null, function (error) {
        paramCallback(error || new Error('Rejected promise'))
    })
} catch (e) {
    paramCallback(e)
}

[dougwilson]

Won't that hurt the performance for the folks who are not using premises, though? We should probably see what the perf cost is going to be between the two methods. We wouldn't want to unnecessarily hurt the perf of folks if not necessary.

[mastermatt]

Good point. It will queue a micro-task either way. For paramCallback it doesn't matter since the result isn't used, but in the layer it would cause next to not be called for an extra tick.

[dougwilson]

Right, I would agree. That and the additional work to create the promise object and associated data. Now, what I don't know is how much it actually hurts :) It's certainly more operations, etc. but how much wall clock time it actually adds is to be seen, lol.

[alexandernanberg]

Seems like this is ready to be merged into v2, unless I'm missing something?

[dougwilson]

You can see above there is a requested change from a reviewer that has not been resolved yet.

[gilly3]

It looks like the only thing @wesleytodd was still waiting for from the requested changes was an improvement to the captured stack trace for better debuggability. And then, that improvement seems to have been shown to make no actual difference in the resulting stack traces.

Are we waiting for more proof that the stack traces either can or cannot be improved? Or is there something else holding this up?

[dougwilson]

Hi @gilly3 thanks for the question. I will take a look at the state of this PR.