Skip to content

ref(replay): Enable no-console rule #6347

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 30, 2022
Merged

ref(replay): Enable no-console rule #6347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/replay/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ module.exports = {
{
files: ['*.ts', '*.tsx', '*.d.ts'],
rules: {
// TODO (high-prio): Go through console logs and figure out which ones should be replaced with the SDK logger
'no-console': 'off',
// TODO (high-prio): Re-enable this after migration
'@typescript-eslint/explicit-member-accessibility': 'off',
// TODO (high-prio): Remove this exception from naming convention after migration
Expand Down
2 changes: 1 addition & 1 deletion packages/replay/src/eventBuffer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class EventBufferCompressionWorker implements IEventBuffer {
try {
stringifiedArgs = JSON.stringify(args);
} catch (err) {
console.error(err);
__DEBUG_BUILD__ && logger.error('[Replay] Error when trying to stringify args', err);
stringifiedArgs = '[]';
}

Expand Down
16 changes: 9 additions & 7 deletions packages/replay/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,14 @@ export class Replay implements Integration {

// TODO(deprecated): Maintain backwards compatibility for alpha users
if (usingDeprecatedCaptureOnlyOnError) {
// eslint-disable-next-line no-console
console.warn(
'[@sentry/replay]: The `captureOnlyOnError` option is deprecated! Please configure `errorSampleRate` instead.',
);
}

if (usingDeprecatedReplaysSamplingRate) {
// eslint-disable-next-line no-console
console.warn(
'[@sentry/replay]: The `replaysSamplingRate` option is deprecated! Please configure `sessionSampleRate` instead.',
);
Comment on lines 182 to 193
Copy link
Member

@Lms24 Lms24 Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the change is good for now but WDYT, should we think about removing these deprecated options? We're still in alpha and soon bumping to 7.x so it might be wort a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I would remove them in a separate PR. I think it makes sense to put this into the 0.x --> 7.x changelog, as people will be most likely to actually read that!

Copy link
Member

@Lms24 Lms24 Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, separate PR sounds good to me. Let's just make sure to keep the Replay team informed about it.

Expand Down Expand Up @@ -1081,7 +1083,7 @@ export class Replay implements Integration {
*/
async runFlush(): Promise<void> {
if (!this.session) {
console.error(new Error('[Sentry]: No transaction, no replay'));
__DEBUG_BUILD__ && logger.error('[Replay] No session found to flush.');
return;
}

Expand Down Expand Up @@ -1113,8 +1115,8 @@ export class Replay implements Integration {
eventContext,
});
} catch (err) {
__DEBUG_BUILD__ && logger.error(err);
captureInternalException(err);
console.error(err);
}
}

Expand All @@ -1135,7 +1137,7 @@ export class Replay implements Integration {
}

if (!this.session?.id) {
console.error('[Replay]: No transaction, no replay');
__DEBUG_BUILD__ && logger.error('[Replay] No session found to flush.');
return;
}

Expand All @@ -1161,7 +1163,7 @@ export class Replay implements Integration {
try {
await this.flushLock;
} catch (err) {
console.error(err);
__DEBUG_BUILD__ && logger.error(err);
} finally {
this.debouncedFlush();
}
Expand Down Expand Up @@ -1306,13 +1308,13 @@ export class Replay implements Integration {
});
this.resetRetries();
return true;
} catch (ex) {
console.error(ex);
} catch (err) {
__DEBUG_BUILD__ && logger.error(err);
// Capture error for every failed replay
setContext('Replays', {
retryCount: this.retryCount,
});
captureInternalException(ex);
captureInternalException(err);

// If an error happened here, it's likely that uploading the attachment
// failed, we'll can retry with the same events payload
Expand Down
9 changes: 3 additions & 6 deletions packages/replay/test/unit/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,8 +525,7 @@ describe('Replay', () => {
const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };

// Suppress console.errors
jest.spyOn(console, 'error').mockImplementation(jest.fn());
const mockConsole = console.error as jest.MockedFunction<typeof console.error>;
const mockConsole = jest.spyOn(console, 'error').mockImplementation(jest.fn());

// fail the first and second requests and pass the third one
mockTransportSend.mockImplementationOnce(() => {
Expand Down Expand Up @@ -576,8 +575,7 @@ describe('Replay', () => {
jest.spyOn(replay, 'sendReplay');

// Suppress console.errors
jest.spyOn(console, 'error').mockImplementation(jest.fn());
const mockConsole = console.error as jest.MockedFunction<typeof console.error>;
const mockConsole = jest.spyOn(console, 'error').mockImplementation(jest.fn());

expect(replay.session?.segmentId).toBe(0);

Expand Down Expand Up @@ -693,8 +691,7 @@ describe('Replay', () => {
it('does not create replay event if recording upload completely fails', async () => {
const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP, type: 3 };
// Suppress console.errors
jest.spyOn(console, 'error').mockImplementation(jest.fn());
const mockConsole = console.error as jest.MockedFunction<typeof console.error>;
const mockConsole = jest.spyOn(console, 'error').mockImplementation(jest.fn());
// fail the first and second requests and pass the third one
mockSendReplayRequest.mockImplementationOnce(() => {
throw new Error('Something bad happened');
Expand Down
2 changes: 2 additions & 0 deletions packages/replay/worker/src/handleMessage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ export function handleMessage(e: MessageEvent): void {
success: false,
response: err,
});

// eslint-disable-next-line no-console
console.error(err);
}
}
Expand Down