[jrandolf]

Motivation

Preserve non-Error thrown values in explicit results for v5, and rename the failure field as requested.

Approach

Use { ok: false, cause: unknown } for failed results. Err() retains the original value without ThrownValueError; runtime consumers and documentation use cause. #1264 adds the v4 alias and deprecates error.

Fix CI by naming the benchmark package to prevent npm's audit crash. Local tests, formatting, lint, type checks, and the Node test build pass.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/effection@1229

commit: a475ca9

[codspeed]

Merging this PR will not alter performance

✅ 6 untouched benchmarks

---

_{Comparing jrandolf:jrandolf/error-unknown (a475ca9) with v4 (c623418)¹}

Footnotes

1. No successful run was found on v5 (b1e1f5f) during the generation of this report, so v4 (c623418) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[cowboyd]

Hey @jrandolf, Thanks for the PR!

This is definitely a breaking change. I'm not opposed to it, but I'm curious why you think it is lossy? All information is retained, and If you are capturing a result object in the first place then by definition you are already preventing code upwards on the stack from seeing the throw value. In order to re-expose the stack to the error, you are would already need to destructure via the .ok guard. It seems to me at the point of de-structuring, you could re-raise any thrown value at the same point.

export function unbox<T>(result: Result<T>): T {
  if (result.ok) {
    return result.value;
  } else {
    throw result.error instanceof ThrowValueError ? result.error.cause : result.error;
  }
}

Or is the problem that Effection itself does not do this when it re-throws an error? That would definitely be a problem

[jrandolf]

Yes. Effection itself rethrows the wrapper.

#1149 established Result.error: Error, added ThrownValueError, and added coverage requiring until(Promise.reject("error")) to expose that wrapper. The behavior is deliberate.

Changing unbox() would fix the paths that call it, including callcc, scope destruction, and task-group shutdown. Other runtime paths bypass it: Do, race, coroutine resumption, the task iterator, the task Promise adapter, and background error propagation all use the normalized result.error directly. Preserving Result.error: Error while restoring the original thrown value would require the same unwrapping rule at each of those boundaries.

This PR changes the common representation instead. Result.error becomes unknown, and Err() stores the exact cause, so explicit results and automatic propagation follow the same rule. The wrapper retains the original information in cause; the behavioral difference is that Effection changes the identity and observable type of the caught value.

SvelteKit shows why that matters. Its HttpError and Redirect are plain classes, and isHttpError() and isRedirect() recognize them with instanceof. Wrapping either value breaks those guards. Next.js does not have this problem because its redirect sentinel is an Error and already passes through unchanged.

[cowboyd]

Right. This could be a change that could be accepted as part of a 5.0 release, but since it breaks a public API, it cannot go into the v4 release series as-is. However, failure to unbox is a bug in v4 and should be fixed.

[cowboyd]

I think the path forward here is to start a v5 branch with this as the first change on it, and then add the unwrapping behavior to v4. Is that latter item something you'd be willing to take on?

[jrandolf]

Let's do it!

[jrandolf]

@cowboyd I opened #1230 with the v4-compatible fix: explicit Result.error values remain Error, while automatic operation, coroutine, task, race, halt, and Promise boundaries restore the original thrown value. The full 235-step suite, lint, formatting, and type checks pass. I left this PR unchanged for the future v5 branch.

[jrandolf]

@cowboyd Will you create the v5 branch? I don't have permission for this.

[cowboyd]

@jrandolf done!

[jrandolf]

@cowboyd This PR is ready!

[cowboyd]

As long as we're making a breaking change, would it make sense to change the name of the field toreason or cause and then, in v4 introduce the new field name as an alias for error and deprecate error?

[jrandolf]

Done

[cowboyd]

Do we need this?

[jrandolf]

Yes. I reproduced the benchmark install failure locally: without a package name, npm's audit crashes with Cannot read properties of null (reading 'name').

[jrandolf]

As long as we're making a breaking change, would it make sense to change the name of the field toreason or cause and then, in v4 introduce the new field name as an alias for error and deprecate error?

I've thought about it and reason/cause can exist even when the Result is ok (or at least, that's the correct semantics). Considering we are doing a breaking change, I think it's reasonable to just change error because semantically it's wrong. Users should ALWAYS defend their errors using type discrimination in the first place.

But this is up to you.