Skip to content

feat: Add maskAllText option#35

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

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

Conversation

@billyvg
Copy link
Copy Markdown
Member

@billyvg billyvg commented Jan 30, 2023 •
edited
Loading

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

@billyvg billyvg force-pushed the feat-native-mask-all-text-option branch 2 times, most recently from 859a016 to a76ff92 Compare February 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 mydea February 3, 2023 19:00
@billyvg billyvg force-pushed the feat-native-mask-all-text-option branch from 18bd320 to 60d7d40 Compare February 3, 2023 19:01
@billyvg
feat: Add maskAllText option
326ab31 
Similar to `maskAllInputs`, this option will mask all text nodes (except those defined by `unmaskSelector`)
@billyvg billyvg force-pushed the feat-native-mask-all-text-option branch from 60d7d40 to 326ab31 Compare February 3, 2023 19:12
@billyvg billyvg mentioned this pull request Feb 6, 2023
Merged
Lms24
Lms24 reviewed Feb 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Lms24 Lms24 left a comment •
edited
Loading

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
Member Author

billyvg commented Feb 7, 2023

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 Lms24 February 8, 2023 14:31
Lms24
Lms24 approved these changes Feb 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Lms24 Lms24 left a comment

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-v1 Feb 8, 2023
@billyvg billyvg deleted the feat-native-mask-all-text-option branch February 8, 2023 18:32
@billyvg
Copy link
Copy Markdown
Member Author

billyvg commented Feb 8, 2023

@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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lms24 Lms24 Lms24 approved these changes

@mydea mydea Awaiting requested review from mydea

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@billyvg @Lms24