Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Revert Errors serialization behavior #2197

Merged
merged 2 commits into from
May 3, 2023

Conversation

yannickadam
Copy link
Contributor

Motivation

Change of behavior when serializing errors:
#2195

Previously, an errror was serialized as an empty object {}. However, additional properties (added manually or from a custom error) were serialized:

const err = new Error('test')
err.myProp = 42

JSON.stringify(err)
=> { myProp : 42 }

Errors fall in the catch-all within the sanitize process, resulting in :

sanitize(err)
=> '[Error]'

Changes

Specific handler for instanceof Error
Serialize additional properties from errors
=> [Error] reverts back to {} (empty object) for errors without additional properties.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@yannickadam yannickadam requested a review from a team as a code owner May 2, 2023 10:20
@codecov-commenter
Copy link

Codecov Report

Merging #2197 (6363cfa) into main (991e974) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2197      +/-   ##
==========================================
+ Coverage   93.81%   93.90%   +0.08%     
==========================================
  Files         201      201              
  Lines        6082     6088       +6     
  Branches     1347     1349       +2     
==========================================
+ Hits         5706     5717      +11     
+ Misses        376      371       -5     
Impacted Files Coverage 螖
packages/core/src/tools/serialisation/sanitize.ts 100.00% <100.00%> (酶)

... and 1 file with indirect coverage changes

馃摚 We鈥檙e building smart automated test selection to slash your CI/CD build times. Learn more

@yannickadam yannickadam force-pushed the yannick.adam/revert_error_serialization_behavior branch from 6363cfa to 1e9cbe7 Compare May 2, 2023 10:53
@yannickadam yannickadam force-pushed the yannick.adam/revert_error_serialization_behavior branch from 1e9cbe7 to 859cdee Compare May 2, 2023 11:44
@yannickadam
Copy link
Contributor Author

/to-staging

@dd-devflow
Copy link

dd-devflow bot commented May 2, 2023

馃殏 Branch Integration

commit 859cdee will soon be integrated into staging-18.

This build is going to start soon! (estimated merge in less than 0s)

you can cancel this operation by commenting your pull request with /to-staging -c!

@dd-devflow
Copy link

dd-devflow bot commented May 2, 2023

馃殏 Branch Integration

commit 859cdee has been merged into staging-18

@dd-devflow dd-devflow bot added the staging-18 label May 2, 2023
// IE11 adds a description field
// Safari IOS12 adds parts of the stack
const error = new Error('My Error')
expect(JSON.stringify(sanitize(error))).toEqual(JSON.stringify(error))
Copy link
Member

Choose a reason for hiding this comment

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

馃 nitpick: 鈥廳id you try using { ...error } or Object.assign({}, error) instead of JSON.stringify? It should yield better message when the assertion fails

Suggested change
expect(JSON.stringify(sanitize(error))).toEqual(JSON.stringify(error))
expect(sanitize(error)).toEqual({ ...error })

@yannickadam yannickadam merged commit 3cbe094 into main May 3, 2023
17 checks passed
@yannickadam yannickadam deleted the yannick.adam/revert_error_serialization_behavior branch May 3, 2023 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants