Uh oh!
There was an error while loading. Please reload this page.
feat(core): Extract scope application to util - #9804
Merged
Conversation
Contributor
size-limit report 📦
|
mydeaforce-pushed
the
fn/apply-scope-to-event
branch
2 times, most recently
from
December 18, 2023 13:46
97db3e4 to
f2749a4Comparemydea
marked this pull request as ready for review
December 18, 2023 13:48
AbhiPrasad
approved these changes
Dec 18, 2023
AbhiPrasad
left a comment
Contributor
There was a problem hiding this comment.
This change feels reasonable enough to me, it's also nice to get the individual functions in applyScopeDataToEvent.ts that we have the flexibility to export and use in the future.
Lms24
approved these changes
Dec 18, 2023
Lms24
left a comment
Member
There was a problem hiding this comment.
Nice, I agree it feels better to have this function outside. Single responsibility principle also agrees with us 😅
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+522
to
+529
| const eventProcessors: EventProcessor[] = [ | ||
| ...additionalEventProcessors, | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| ...getGlobalEventProcessors(), | ||
| ...this._eventProcessors, | ||
| ]; | ||
| return notifyEventProcessors(eventProcessors, event, hint); |
Member
There was a problem hiding this comment.
just to confirm since we deprecated this function: The same logic here is already present in places calling applyScopeDataToEvent, right?
MemberAuthor
There was a problem hiding this comment.
yes, the behavior remains the same here as before!
mydeaforce-pushed
the
fn/apply-scope-to-event
branch
3 times, most recently
from
December 19, 2023 12:56
97ea3ec to
0a4b49fCompareCo-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
mydeaforce-pushed
the
fn/apply-scope-to-event
branch
from
December 19, 2023 13:25
0a4b49f to
e619908Compare
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is a try to move the application of scope data to events out of the scope class into a util function.
This changes the flow to be:
getScopeData()method that returns data that should be applied to events.prepareEventmethod uses that to actually apply scope data to the event etc.I think this makes sense but would appreciate some more eyes on this as well. For me the separation makes also more sense, as e.g. the application of event processors etc. should not really be tied to the scope at all.
This is also is a first step to then allow us to add global scopes more easily.