Skip to content

Serialize non-error cause - #204

Open
Andreyco wants to merge 3 commits into
pinojs:masterfrom
Andreyco:master
Open

Serialize non-error cause#204
Andreyco wants to merge 3 commits into
pinojs:masterfrom
Andreyco:master

Conversation

@Andreyco

@Andreyco Andreyco commented Jun 12, 2026

Copy link
Copy Markdown

Issues:

  • The non-error cause property set via constructor is not retained.
  • The non-error "cause" is not included in the stack

Why is that happening:

  • Serializers have logic that only allows error like "cause", while runtime is very permissive (in fact, it's typed as { cause?: unknown }
  • In addition the cause property set via the constructor is non-enumerable. The serializers were primarily using a for...in loop to find extra properties, which skips non-enumerable ones.

Why the change:

  • while one should not, it's possible to throw 42
  • but it perfectly ok to Promise.reject(42)
  • it partially supported by the lib (via err.cause = 42, but not via Error('ooops', { cause: 42 }))

I really hope per commit "evolution" helps in review process.

@Andreyco
Andreyco force-pushed the master branch 2 times, most recently from a968b55 to 3aba4c4 Compare June 12, 2026 10:50
@Andreyco Andreyco changed the title Verify explicit error cause chain Serialize non-error cause Jun 12, 2026
@Andreyco

Andreyco commented Jun 12, 2026

Copy link
Copy Markdown
Author

I also consider presence of cause on SerializedError to be an issue.
Can solve that, however would break existing test suite and want to prevent mixing things.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina
mcollina requested a review from jsumners July 1, 2026 14:15

@jsumners jsumners left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should promote handling of not errors.

@mcollina

mcollina commented Jul 1, 2026

Copy link
Copy Markdown
Member

Neither do I tbh. @Andreyco what does it currently happen?

@Andreyco

Andreyco commented Jul 2, 2026

Copy link
Copy Markdown
Author

hello everyone, thanks for finding time to respond...

I don't think we should promote handling of not errors.

Neither do I tbh. @Andreyco what does it currently happen?

Basically, Promises rejected with non error as reason is the motivation for this PR. It's way too common among 3rd party packages.

If you are 100% against and won't accept non error-like variant, let me reiterate on this PR, as there are some internal changes preventing me from writing custom wrapper around for error serializer (afaik cause not being preserved, it's some time since I wrote this)

Waiting for your response before acting. TY!

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.

3 participants