Skip to content

fix: verify trailers#433

Merged
ronag merged 1 commit intomasterfrom
check-trailers
Sep 24, 2020
Merged

fix: verify trailers#433
ronag merged 1 commit intomasterfrom
check-trailers

Conversation

@ronag
Copy link
Member

@ronag ronag commented Sep 23, 2020

Fixes: #432

@ronag ronag requested a review from mcollina September 23, 2020 22:36
class TrailerMismatchError extends UndiciError {
constructor (message) {
super(message)
Error.captureStackTrace(this, TrailerMismatchError)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But UndiciError extends Error, so isn't .stack generated automatically?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually unsure why this is necessary but given the best practices I've been able to read it seems to be the correct way...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the example from MDN:

class CustomError extends Error {
  constructor(foo = 'bar', ...params) {
    // Pass remaining arguments (including vendor specific ones) to parent constructor
    super(...params)

    // Maintains proper stack trace for where our error was thrown (only available on V8)
    if (Error.captureStackTrace) {
      Error.captureStackTrace(this, CustomError)
    }

    this.name = 'CustomError'
    // Custom debugging information
    this.foo = foo
    this.date = new Date()
  }
}

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag merged commit e342818 into master Sep 24, 2020
ronag added a commit that referenced this pull request Sep 24, 2020
@Uzlopak Uzlopak deleted the check-trailers branch February 21, 2024 12:39
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 this pull request may close these issues.

Do we need to verify trailers?

3 participants