Refactor getComponentName function #1
Merged
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
There is no need to throw Error object just to get error stack, it's enough to just create Error object and use it. Additionally, Error.prototype.stack is non-standard so there might be a situation where stack is undefined, so I added test for this case.
|
Great catch & great work! Thank you for your contribution! nit, non-blocker: it would be also nice to add a small test to cover a stack parsing (line 81) as well since coverage ignore is removed; it was not added initially just because it's a little bit 'hacky' way to gather component name via non-standard |
dolfbarr
approved these changes
Dec 22, 2022
|
I can take a look tomorrow into parsing part👍 |
- Added test where error.stack has some format that getComponentName doesn't expect - Changed the return value from "undefined" to "" in such situations
dolfbarr
approved these changes
Dec 23, 2022
|
LGTM! Great work! |
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.
Why?
I noticed this code:
I think there is no need to throw Error object just to get error stack, it's enough to create Error object and use it, like so:
Additionally, Error.prototype.stack is non-standard and there might be a situation where error.stack is
undefined, so I added test for this case (previously it was ignored from coverage). In order to do that, I had to move error as optional first argument to this function. There might be other ways to do it, but this was the easiest one (couldn't correctly override global Error constructor)