-
Notifications
You must be signed in to change notification settings - Fork 10
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
Auto capture console logs #78
Conversation
Updated jest dev dependencies and snapshots to resolve npm errors. |
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.
One comment about configuration
637de7a
to
f5707fe
Compare
After discussion - we discovered new requirements: https://docs.google.com/document/d/1QAGpjrC96ePbY_-SugOYK9zFSaqh1HnkXh6CP5MrRp8 |
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.
Overall seems fine. Added my nit feedback to a stacked pr: #83
Two open issues:
- do we need to expose the safeStringify or can we use an OSS solution? Longer term if we want to maintain this functinality it seems best to do as a separate lib.
- It's unclear why we default to enable and not let the FSTA settings determine the initial state.
Assert = 5, // Clamps to Error on Android | ||
} | ||
|
||
const consoleLevelMap = { |
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.
web supports assert
here as well.
expect(oldConsoleError).toEqual(console.error); | ||
}); | ||
|
||
it('disableConsole does not overwrite third party overwrites', () => { |
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.
+1
// default ON | ||
consoleWatcher.enable(); |
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.
This is confusing, web-capture doesn't default to ON. We default to disabled and only enable when there is a flags.ConsoleWatcher
set to true on the PageResponse. The only exception is the FS.log
API which adds logs.
src/logging/consoleCapture.ts
Outdated
// @ts-expect-error | ||
const isTurboModuleEnabled = global.__turboModuleProxy != null; |
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.
Seems like you need to define a module for the global scope instead of just ignoring the TS errors.
This should work.
declare global {
let __turboModuleProxy: boolean;
}
const isTurboModuleEnabled = __turboModuleProxy != null;
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've added this to my pr feedback branch.
src/logging/consoleCapture.ts
Outdated
// A naive version of ConsoleWatcher on web. | ||
// See https://github.com/cowpaths/mn/blob/865353f374b687e481d3b230e7eec8e1d7be2eb4/projects/fullstory/packages/recording/src/consolewatcher.ts |
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.
this is a public repo correct? doesn't seem that useful to ref our private repo
@@ -0,0 +1,174 @@ | |||
import { safeStringify } from '../logging/safeStringify'; |
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.
since this is a public repo seems like we could add a third-party dep for this instead of managing this directly.
Wiz Scan Summary
|
23dcb27
to
ac6011c
Compare
Closing this for now since we'll need to discuss console scrubbing on the native side |
Issue: https://fullstory.atlassian.net/browse/MOCA-8076
Add methods
enableConsole
anddisableConsole
to FullStory API that will extend React Native'sconsole
API. Messages will be passed toFS.log
.Console capture defaults to enabled, and follows the FS default log level of
info
if the customer hasn't configured it already.Example session: https://app.staging.fullstory.com/ui/KWH/session/6727898337050624:5966357937455104/devtools/console
Added enable and disable API.
Added
safeStringify
from web side, minusDOM
node handling since that's not defined in RN.