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

"Cannot call method 'then' of undefined" when specifying an invalid .catch filter #318

Closed
joepie91 opened this issue Sep 9, 2014 · 5 comments

Comments

@joepie91
Copy link

@joepie91 joepie91 commented Sep 9, 2014

I've written the following code (Coffeescript):

Promise.try ->
    req.model("AuditLogEntry").getAll([], start: start, limit: limit, require: yes)
.catch req.db.EmptyResponse, (err) ->
    throw util.NotFound()
.then (entries) ->
    res.send entries.json()

... and I made the mistake of trying to filter by req.db.EmptyResponse (which was undefined) rather than by the correct attribute req.db.EmptyError. However, bluebird didn't remark on me passing undefined as filter - instead, it gave me the following error:

TypeError: Cannot call method 'then' of undefined
    at module.exports (/home/sven/projects/anonnews3/routes/audit-log.js:27:6)
    at handleReturn (/home/sven/projects/anonnews3/node_modules/express-promise-router/lib/express-promise-router.js:27:27)
    at Object.handle (/home/sven/projects/anonnews3/node_modules/express-promise-router/lib/express-promise-router.js:55:9)
    at next_layer (/home/sven/projects/anonnews3/node_modules/express/lib/router/route.js:103:13)
    [...]

This refers to the .then call in line 5 of the code snippet above. Instead of throwing a fairly cryptic error (why does .catch return undefined?), it should probably throw an error of some sort that the specified filter is invalid - after all, it's not an Error type nor a function.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 11, 2014

You are right, I think it should just throw synchronously

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Sep 12, 2014

Not sure I'm particularly fond of the idea of this promise returning function throwing synchronously.

However, it's consistent with what the promise constructor does and it's a core library method rather than an API.

@domenic
Copy link

@domenic domenic commented Sep 12, 2014

Noooooooo

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 13, 2014

Domenic it's very comparabale to syntaxerror, like you would never have a runtime dynamic value as a catch predicate. Or would you?

@domenic
Copy link

@domenic domenic commented Sep 14, 2014

You very well could, e.g. I could imagine some form validation code composing its filter.

In general .then and .catch should never throw. I think it's a mistake that new Promise can do so but I let that one slip through :-/.

It should just return a promise rejected with a TypeError IMO.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.