ref: Enforce stackParser through options. #4953
Merged
Conversation
This file contains hidden or 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
AbhiPrasad
requested review from
a team,
lforst and
Lms24
and removed request for
a team
AbhiPrasad
force-pushed
the
v7-abhi-stack-parser
branch
2 times, most recently
from
f6a0477 to
b9af9dc
Compare
size-limit report 📦
|
AbhiPrasad
force-pushed
the
v7-abhi-stack-parser
branch
from
b9af9dc to
7f366db
Compare
Comment on lines
+34
to
+39
| * Gets a stack parser implementation from Options.stackParser | ||
| * @see Options | ||
| * | ||
| * If options contains an array of line parsers, it is converted into a parser | ||
| */ | ||
| export function stackParserFromOptions(options: StackParserOptions | undefined): StackParser { | ||
| if (options) { | ||
| if (Array.isArray(options.stackParser)) { | ||
| options.stackParser = createStackParser(...options.stackParser); | ||
| } | ||
|
|
||
| if (typeof options.stackParser === 'function') { | ||
| return options.stackParser; | ||
| } | ||
| export function stackParserFromOptions(stackParser: StackParser | StackLineParser[]): StackParser { |
There was a problem hiding this comment.
After this change, seems like it's not being gotten from options anymore. Can we change the name to reflect that?
There was a problem hiding this comment.
This is a good point, and has got me thinking if we even should expose passing in a StackLineParser[], and enforce that it is always an (optional) StackParser.
There was a problem hiding this comment.
The only plus to allow passing of StackLineParser[] is that users can configure stack parsing without needing to import and use createStackParser from @sentry/utils.
Some common lints warn if you use modules that are not direct dependencies so users would also need to add @sentry/utils to their depndencies or we would need to be re-export it from every sdk.
Allowing user passing of StackParser allows completely overridding the entire stack parsing logic. I'm not sure we have a use case for this yet.
timfish
approved these changes
Apr 20, 2022
As stackParser will always be defined in client options, we can remove the usage of `stackParserFromOptions` outside of SDK init.
AbhiPrasad
force-pushed
the
v7-abhi-stack-parser
branch
from
f5a86ee to
503ea25
Compare
Lms24
pushed a commit
that referenced
this pull request
Apr 26, 2022
As stackParser will always be defined in client options, we can remove the usage of `stackParserFromOptions` outside of SDK init.
lobsterkatie
pushed a commit
that referenced
this pull request
Apr 26, 2022
As stackParser will always be defined in client options, we can remove the usage of `stackParserFromOptions` outside of SDK init.
lobsterkatie
pushed a commit
that referenced
this pull request
Apr 26, 2022
As stackParser will always be defined in client options, we can remove the usage of `stackParserFromOptions` outside of SDK init.
AbhiPrasad
added a commit
that referenced
this pull request
May 30, 2022
As stackParser will always be defined in client options, we can remove the usage of `stackParserFromOptions` outside of SDK init.
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.
As stackParser will always be defined in client options, we can remove
the usage of
stackParserFromOptionsoutside of SDK init.cc @timfish