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

Interface changes #104

Open
jonchurch opened this issue Mar 8, 2024 · 4 comments
Open

Interface changes #104

jonchurch opened this issue Mar 8, 2024 · 4 comments

Comments

@jonchurch
Copy link
Member

jonchurch commented Mar 8, 2024

Based on #103

Users likely expect to be able to pass custom properties to errors when creating them via the shortcut methods such as createError.NotFound().

Proposed change would look something like:

const createError = require('http-errors')

createError.NotFound({message: "Aw shucks", data: { foo: "bar" } }) // does not work today

You need to use the createError({...}) form to pass custom properties to an error.

createError(404, {message: "Aw shucks", data: { foo: "bar" } })

There's a few caveats with the current implementation, namely you can't set status via properties.

createError({ status: 404, message: "Aw shucks", data: { foo: "bar" } })
// status is ignored, will create a 500 server error with provided message and data fields

There is also a very high arity for createError, which should be locked down into a stable interface.

(code)

for (var i = 0; i < arguments.length; i++) {
    var arg = arguments[i]
    var type = typeof arg
    if (type === 'object' && arg instanceof Error) {
      err = arg
      status = err.status || err.statusCode || status
    } else if (type === 'number' && i === 0) {
      status = arg
    } else if (type === 'string') {
      msg = arg
    } else if (type === 'object') {
      props = arg
    } else {
      throw new TypeError('argument #' + (i + 1) + ' unsupported type ' + type)
    }
  }

Any of these changes would be semver major. There aren't plans for a major release of http-errors yet, but these are things Im thinking about.

why did I put these both in one issue?

These are both topics related to the interfaces exposed by the library for accepting arguments, which can use some rethinking.

@jonchurch
Copy link
Member Author

There's been prior discussions around the semantics of the inputs we accept, and how it impacts the outputs:

#58 #37 and likely more

@wesleytodd
Copy link
Member

Any of these changes would be semver major.

I think it was mentioned in one of the issues that it would be right? FWIW I think there is nothing wrong with packages like this working ahead and doing major releases as long as we are clear on the minimum support from the LTS strat as mentioned in expressjs/discussions#210

@jonchurch
Copy link
Member Author

jonchurch commented Mar 12, 2024

Yes, Im saying that any of these changes would indeed be semver major.

Im in no rush to make this change or the subclassing change.

This is a tracking issue for a change which is...

related to the interfaces exposed by the library for accepting arguments, which can use some rethinking.

if we did want to land a change in this category, it would need to be in a major.

From reading issue history since posting this though, this seems to be incorrect:

There aren't plans for a major release of http-errors yet, but these are things Im thinking about.

@dougwilson mentioned in a comment that he was planning to make all properties nonenumerable in v3, but that's the only v3 ref Ive run into so far.

There aren't any plans for a new major at least in that it's not documented in the repo or issues so far as I can tell.

@jonchurch jonchurch changed the title Pass custom properties to shortcut constructors, reduce arity Interface changes Mar 17, 2024
@jonchurch
Copy link
Member Author

Related #96 #78

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

2 participants