Skip to content

feat: Add maskAllText option - #35

Merged
billyvg merged 1 commit into
sentry-v1from
feat-native-mask-all-text-option
Feb 8, 2023
Merged

feat: Add maskAllText option#35
billyvg merged 1 commit into
sentry-v1from
feat-native-mask-all-text-option

Conversation

@billyvg

@billyvgbillyvg commented Jan 30, 2023

Copy link
Copy Markdown
Member

Similar to maskAllInputs, this option will mask all text nodes (except those defined by unmaskTextSelector)

@billyvg
billyvgforce-pushed the feat-native-mask-all-text-option branch 2 times, most recently from 859a016 to a76ff92CompareFebruary 2, 2023 21:34
@billyvg
billyvg marked this pull request as ready for review February 3, 2023 16:13
@billyvg
billyvg requested review from Lms24 and mydeaFebruary 3, 2023 19:00
@billyvg
billyvgforce-pushed the feat-native-mask-all-text-option branch from 18bd320 to 60d7d40CompareFebruary 3, 2023 19:01
Similar to `maskAllInputs`, this option will mask all text nodes (except those defined by `unmaskSelector`)

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On first glance this looks good to me, but I have a question:
I probably missed an important part of the conversation here, but why do we need this instead of or over the maskTextSelector option being set to MASK_ALL_TEXT_SELECTOR in the SDK? Is it because otherwise we can't mask attributes (i.e. #40)?

@billyvg

Copy link
Copy Markdown
MemberAuthor

On first glance this looks good to me, but I have a question:

I probably missed an important part of the conversation here, but why do we need this instead of or over the maskTextSelector option being set to MASK_ALL_TEXT_SELECTOR in the SDK? Is it because otherwise we can't mask attributes (i.e. #40)?

Yeah masking attributes is one of the reasons. Other is that the selector was kind of a hack to do the masking we wanted and I'd rather have it done natively in rrweb without using a separate selector. We saw it previously using '*' that it could lead to unintended side effects.

@billyvg
billyvg requested a review from Lms24February 8, 2023 14:31

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just as in #40 I'm curious about the declaration changes. Not sure why they occur.
We changed the target language level a few days ago, right? Maybe that's it? Regardless, I think that's fine

@billyvg
billyvg merged commit faaf617 into sentry-v1Feb 8, 2023
@billyvg
billyvg deleted the feat-native-mask-all-text-option branch February 8, 2023 18:32
@billyvg

Copy link
Copy Markdown
MemberAuthor

@Lms24 Yeah that's what i'm thinking, it could have been a merge artifact... looking at the other exported types, none of them have declare either.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@billyvg@Lms24