Fix evaluateCause loosing access to err
#109
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsumners
requested changes
Jun 13, 2022
|
|
||
| return cause instanceof Error | ||
| ? cause | ||
| : undefined | ||
| } | ||
|
|
||
| /** | ||
| * @param {unknown|(()=>err)} cause | ||
| * @param {Error|{ cause?: unknown|(()=>err)}} err |
There was a problem hiding this comment.
evaluateCause now takes the same argument as getErrorCause
There was a problem hiding this comment.
I completely missed we have that jsdoc. I don't think it is even valid.
There was a problem hiding this comment.
I can't tell, haven't used JSDoc for a long time.
KoltesDigital
force-pushed
the
fix-cause-this
branch
from
30d7481 to
08a0499
Compare
|
Agreed for the test, here it is. It fails before the fix. |
KoltesDigital
changed the title
evaluateCause loosing access to thisevaluateCause loosing access to err
jsumners
reviewed
Jun 13, 2022
KoltesDigital
force-pushed
the
fix-cause-this
branch
2 times, most recently
from
01cbb10 to
20c44ec
Compare
|
@voxpelli please take a look at this. |
KoltesDigital
force-pushed
the
fix-cause-this
branch
from
20c44ec to
3f81ce6
Compare
voxpelli
added a commit
to voxpelli/pino-std-serializers
that referenced
this pull request
Jun 13, 2022
Instead rely on its content being appended to the message and the stack. This is an alternative fix to pinojs#94, replacing pinojs#105 and pinojs#108 + makes pinojs#109 not needed
|
I added an alternate solution to it, which removed the new competing serialization system and relies on the original summary system: #110 |
mcollina
pushed a commit
that referenced
this pull request
Jun 13, 2022
…ead (#110) * Revert "Nested VError causes (#108)" This reverts commit 5b88f7e. * Revert "Serialize errors with cause (#105)" This reverts commit ae83956. * Never serialize `.cause` when an `Error` Instead rely on its content being appended to the message and the stack. This is an alternative fix to #94, replacing #105 and #108 + makes #109 not needed * Add tests * Add explainer comment
|
This PR can be closed now that #110 has been merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR:
erris not bound asthisto thecausemethod when callingevaluateCause. The call tocauseexecutesError.prototype.cause(or its overload), butthisrefers to the global object, whereas it should beerr. This leads to errors as soon asthisis used in thecausemethod implementation.This PR fixes this behavior.