-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(logrocket): add request sanitizing and add username& email to sessions #2662
Conversation
👇 Click on the image for a new way to code review
Legend |
@@ -38,9 +38,31 @@ import { BrandPage } from './pages/brand/BrandPage'; | |||
import { SegmentProvider } from './store/segment.context'; | |||
import LogRocket from 'logrocket'; | |||
import setupLogRocketReact from 'logrocket-react'; | |||
import packageJson from '../package.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you had a chance to check if in some case this is affecting the build/dist folder structure of the web?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cliftonz his suspicion was that the package.json
won't be available after the build... but I've checked it and actually the package.json
file is bundled to the app as the JSON string and then parsed... it just adds ~5.14kb to the bundled main JS chunk...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LetItRock is the 5kb going to be an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scopsy This should be good to merge then
…e to logrockets limits
Co-authored-by: Paweł Tymczuk <LetItRock@users.noreply.github.com>
@Cliftonz let's fix the prettier errors and feel free to merge once test passed 🎉 |
No description provided.