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

params overloading #4

Closed
rlidwka opened this issue Sep 9, 2014 · 10 comments
Closed

params overloading #4

rlidwka opened this issue Sep 9, 2014 · 10 comments
Assignees

Comments

@rlidwka
Copy link
Contributor

rlidwka commented Sep 9, 2014

Is there any point in doing create(404, 'message') when you can use create[404]('message') ?

Also, you can do create(404, 'message') as well as create('message', 404), which will be pain to support if we decide to change that.

I'm now thinking about literally shadowing global Error object with this one:

var Error = require('http-error')
throw new Error('this would work as usual')
throw new Error[404]('this would work as http error')

... and things like treating numbers as status codes might break the idea.

@Fishrock123
Copy link
Member

I think we should have just one syntax. That last bit is an interesting idea, but how might it play with other things?

@jonathanong
Copy link
Member

i'd much rather support create(404, 'message') vs. using []s anywhere. it was mainly for backawrds compat for me because i hijacked someone else's module

@rlidwka
Copy link
Contributor Author

rlidwka commented Sep 9, 2014

i'd much rather support create(404, 'message')

Ohh... okay. But what about not supporting create('message', 404)?

You can even do funny stuff like create(403, 404, 500) right now, which is equivalent to create(500).

@jonathanong
Copy link
Member

lol why you do that. it's mostly because we support errors and stuff too and i ended up with like 4 if statements then gave up

@rlidwka
Copy link
Contributor Author

rlidwka commented Sep 10, 2014

That last bit is an interesting idea, but how might it play with other things?

Well... I'm gonna try that: rlidwka/sinopia@a5cd498

If it fails horribly, I'll let you know. :P

@chazmo03
Copy link

What if you also overload the factories to accept Error objects instead of just messages? This way you add the status and pass along the error.

something.do(function(err) {
    if (err) { return next(create[400](err)); }
    ...
}

@dougwilson
Copy link
Contributor

@chazmo03 we would only do that if you could do the same for TypeError, etc. Those are not factories, but classes.

@chazmo03
Copy link

@dougwilson Looking into the code, index.js:19, it looks like this is already possible, just not documented. Running the following seems to create errors with appropriate codes but with different messages. Am I missing something?

createError[400]('Err!').message
createError[400](new Error('Err!')).message
createError[400](new TypeError('Err!')).message

@dougwilson
Copy link
Contributor

Um, line 19 is the factory. You are not calling that factory, because you are doing createError[400], which is a class. You want to be doing createError(400, new Error('Err!')) to use the factory.

@dougwilson dougwilson self-assigned this Feb 13, 2017
@dougwilson
Copy link
Contributor

I added a deprecation to the one thing everyone agreed upon here: taking the status code as the non-first argument in the factory function. We'll see if there is heavy push back when it gets published.

@dougwilson dougwilson mentioned this issue Feb 14, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants