Make unwrap exception test friendly

[bluenote10]

This would be my shot at the other action item regarding #211.

At first I tried to use a custom UnwrapError which inherits from Error, and attach the extra value/error there. The problem is that Jest seems to only only ever show the message of an exception as soon as the instanceof Error check is true. Not sure if there is a way around it. With this behavior of Jest the only option would have been to attach this.value / this.error to the message itself, but that sucks because, because it would most likely require some JSON.stringify, and it prevents the final printing from pretty printing the object properly.

Then I figured: The _unsafeUnwrap technically doesn't really need to throw an error that is instance of Error. Perhaps it even shouldn't. It is not something this is supposed to be caught in normal code anyway, so the solution can focus on the test case. It turns out that simply throwing a plain object gives very nice test output. Examples:

it('Makes nice unwrap error 1', () => {
  const okVal = err(42)
  expect(okVal._unsafeUnwrap()).toBe('whatever')
})

it('Makes nice unwrap error 2', () => {
  const okVal = ok({ foo: ['some', 'more', 'complex', 'data'], bar: [1, 2, 3, 4] })
  expect(okVal._unsafeUnwrapErr()).toBe('whatever')
})

What do you think?

[supermacro]

Hey @bluenote10, my thoughts are that keeping the stack trace is useful, especially if someone knowingly uses either _unsafeUnwrap or _unsafeUnwrapErr in a non-test scenario (even if it's vehemently discouraged).

Secondly, there's a trick that one can use in JSON.stringify in order to pretty-print complex objects. Here is the code:

throw new Error('Called `_unsafeUnwrapErr` on an Ok containing:\n' + JSON.stringify(this.value, null, 2))

Which leads to the following example output in Jest:

  ● BLAH › Fails

    Called `_unsafeUnwrap` on an Err containing:
    {
      "a": "Hello",
      "b": {
        "name": "Gio",
        "favoriteNumbers": [
          1,
          2,
          3,
          {
            "special": {
              "nested": {
                "object": true
              }
            }
          }
        ]
      }
    }

      129 | 
      130 |   _unsafeUnwrap(): T {
   > 131 |     throw new Error('Called `_unsafeUnwrap` on an Err containing:\n' + JSON.stringify(this.error, null, 2))
             |           ^
      132 |   }
      133 | 
      134 |   _unsafeUnwrapErr(): E {

      at Err.Object.<anonymous>.Err._unsafeUnwrap (src/result.ts:131:11)
      at Object.<anonymous> (tests/index.test.ts:14:30)

So I think this is the path I'm going to take.

[supermacro]

See here:

388b4d1

[bluenote10]

To my knowledge it is a bit of an anti-pattern to do JSON.stringify(this.value, null, 2) in an exception message. What if the data is huge, leading to a string many MBs in size? I was told: If you want to pass out data, leave it as is, and attach it to the object. Otherwise the user doesn't have an option to opt out from generating the huge string. From a testing perspective it would work of course, but it feels a bit wrong to me.

[supermacro]

Going to close this issue seeing that there's a implementation in master now, but let me know if I'm missing something or it could be improved!

[supermacro]

You have a good point, I suppose that this data could be logged out in the error message automatically in test environments only.

[bluenote10]

my thoughts are that keeping the stack trace is useful

I might still be possible to attach a stack trace when throwing the object via something like {message: ..., value: ..., stack: (new Error().stack)}, but I haven't tested this enough.

[supermacro]

I was actually playing around with this approach originally, but figured having a native Error might be more "idiomatic" or less surprising for a user (i.e. in a codebase that does e instanceof Error check for some sort of catch-all error handling).

[bluenote10]

You have a good point, I suppose that this data could be logged out in the error message automatically in test environments only.

Yes but the memory overhead is still there. If you call .unsafeUnwrap() in the wrong spot in production code, it will still have to render a potentially huge string internally. Of course users should not (edit, sorry ;)) rely on calling .unsafeUnwrap() and catching it in the first place ;) (which is why not inheriting from Error might even be more in the spirit of neverthrow, similar to erroneous unwraps in Rust are panics) but enforcing the stringification internally still doesn't feel so perfect to me.

[supermacro]

I'm going to give it some thought and then decide what changes to make before releasing this change (probably tomorrow).

I was thinking maybe something like this:

  _unsafeUnwrapErr(): E {
    const additionalInfo = isTestEnvironment()
        ? ' containing:\n' + JSON.stringify(this.value, null, 2)
        : ''
    throw new Error('Called `_unsafeUnwrapErr` on an Ok' + additionalInfo)
  }

However, this ends up being bad design in my opinion since the behaviour changes based on the environment that it runs in.

Thus, I think it does make sense to not have a Error object at all like you originally suggested, as long as a stack trace can still be captured.