Skip to content

🐛 unwrap non-Error runtime failures - #1230

Open
jrandolf wants to merge 2 commits into
thefrontside:v4from
jrandolf:jrandolf/unbox-thrown-values
Open

🐛 unwrap non-Error runtime failures#1230
jrandolf wants to merge 2 commits into
thefrontside:v4from
jrandolf:jrandolf/unbox-thrown-values

Conversation

@jrandolf

Copy link
Copy Markdown

Motivation

Effection v4 deliberately keeps Result.error typed as Error, wrapping non-Error causes in ThrownValueError. Automatic propagation currently rethrows or rejects that wrapper, so caught values lose their original identity and runtime guards such as instanceof fail. This is the v4-compatible follow-up discussed in #1229.

Approach

  • keep explicit failed Result values wrapped as Error
  • restore the original cause when failures cross operation, coroutine, task, race, halt, and Promise boundaries
  • cover promise rejections, races, background child failures, and direct unbox() behavior

Validation: deno fmt, deno lint, deno check mod.ts experimental.ts, and the full 235-step test suite.

@pkg-pr-new

pkg-pr-new Bot commented Aug 31, 2026

Copy link
Copy Markdown

Open in StackBlitz

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

commit: 58a048d

@codspeed-hq

codspeed-hq Bot commented Aug 31, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 22.68%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 5 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Memory effection-inline.recursion 4.7 KB 6.1 KB -22.68%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing jrandolf:jrandolf/unbox-thrown-values (58a048d) with v4 (70ddc1c)

Open in CodSpeed

@cowboyd

cowboyd commented Aug 31, 2026

Copy link
Copy Markdown
Member

@jrandolf Looks good.

There is one potential regression: I don't think that the type of Exit in main() is safe any more https://github.com/thefrontside/effection/blob/v4/lib/main.ts#L167

Whereas before, we knew all throws would be boxed in an error, we don't anymore because we could (and should) see the original cause. I think now Exit.error should be more properly typed as unknown, not Error | undefined

As it stands, if someone throws "null" or "0" or "false" it won't print anything. A minor thing, but it should actually simplify the type and remove the need for the type assertion on https://github.com/thefrontside/effection/blob/v4/lib/main.ts#L139

GitHub
Structured concurrency and effects for JavaScript. Contribute to thefrontside/effection development by creating an account on GitHub.
GitHub
Structured concurrency and effects for JavaScript. Contribute to thefrontside/effection development by creating an account on GitHub.

@jrandolf

Copy link
Copy Markdown
Author

@cowboyd Fixed in 26587dc: Exit.error is now unknown, the assertion is gone, and main() checks for the property so falsy causes such as false, 0, and null are still printed. I added a throw false regression; formatting, lint, type checks, and all 208 core test steps pass.

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.

2 participants