Skip to content
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

Remove console log usage #10341

Merged
merged 7 commits into from
Jun 7, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions news/3 Code Health/10202.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove usage of console.log in renderers.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { KernelMessage } from '@jupyterlab/services';
import * as uuid from 'uuid/v4';
import { Event, EventEmitter, NotebookDocument } from 'vscode';
import type { Data as WebSocketData } from 'ws';
import { traceInfoIfCI, traceVerbose, traceError } from '../../platform/logging';
import { traceVerbose, traceError, traceInfo } from '../../platform/logging';
import { IDisposable } from '../../platform/common/types';
import { Deferred, createDeferred } from '../../platform/common/utils/async';
import { noop } from '../../platform/common/utils/misc';
Expand Down Expand Up @@ -106,7 +106,7 @@ export class IPyWidgetMessageDispatcher implements IIPyWidgetMessageDispatcher {
public receiveMessage(message: IPyWidgetMessage): void {
switch (message.message) {
case IPyWidgetMessages.IPyWidgets_logMessage:
traceInfoIfCI(`Widget Message: ${message.payload}`);
traceInfo(`Widget Message: ${message.payload}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this traceInfo because after these two changes, this is really only stuff that we actually want to show to the user (or us when request logs).

break;
case IPyWidgetMessages.IPyWidgets_Ready:
this.sendKernelOptions();
Expand Down
1 change: 0 additions & 1 deletion src/webviews/extension-side/variablesView/variableView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ export class VariableView extends WebviewViewHost<IVariableViewPanelMapping> imp
this.documentManager.onDidChangeActiveTextEditor(this.activeTextEditorChanged, this, this.disposables);

this.dataViewerChecker = new DataViewerChecker(configuration, appShell);
console.log(`Done initializing variables`);
}

@captureTelemetry(Telemetry.NativeVariableViewLoaded)
Expand Down
6 changes: 0 additions & 6 deletions src/webviews/webview-side/ipywidgets/common/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { KernelSocketOptions } from '../../../../kernels/types';
import { Deferred, createDeferred } from '../../../../platform/common/utils/async';
import { serializeDataViews, deserializeDataViews } from '../../../../platform/common/utils/serializers';
import { IInteractiveWindowMapping, IPyWidgetMessages } from '../../../../platform/messageTypes';
import { logMessageOnlyOnCI } from '../../react-common/logger';
import { IMessageHandler, PostOffice } from '../../react-common/postOffice';

/* eslint-disable @typescript-eslint/no-explicit-any */
Expand Down Expand Up @@ -324,7 +323,6 @@ class ProxyKernel implements IMessageHandler, Kernel.IKernelConnection {
this.messageHooks.set(msgId, hook);

// Wrap the hook and send it to the real kernel
logMessageOnlyOnCI(`Registering hook for ${msgId}`);
this.realKernel.registerMessageHook(msgId, this.messageHook);
}

Expand All @@ -350,7 +348,6 @@ class ProxyKernel implements IMessageHandler, Kernel.IKernelConnection {
this.lastHookedMessageId = undefined;

// Remove from the real kernel
logMessageOnlyOnCI(`Removing hook for ${msgId}`);
this.realKernel.removeMessageHook(msgId, this.messageHook);
}

Expand Down Expand Up @@ -393,9 +390,6 @@ class ProxyKernel implements IMessageHandler, Kernel.IKernelConnection {
}
private messageHookInterceptor(msg: KernelMessage.IIOPubMessage): boolean | PromiseLike<boolean> {
try {
logMessageOnlyOnCI(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured if we needed these later, we can add a new IPYWidgets_LogMessageIfCI message. Then on the other side it would only log these if running on CI.

`Message hook callback for ${(msg as any).header.msg_type} and ${(msg.parent_header as any).msg_id}`
);
// Save the active message that is currently being hooked. The Extension
// side needs this information during removeMessageHook so it can delay removal until after a message is called
this.lastHookedMessageId = msg.header.msg_id;
Expand Down
9 changes: 5 additions & 4 deletions src/webviews/webview-side/ipywidgets/kernel/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
IInteractiveWindowMapping,
InteractiveWindowMessages
} from '../../../../platform/messageTypes';
import { logMessage } from '../../react-common/logger';

class WidgetManagerComponent {
private readonly widgetManager: WidgetManager;
Expand Down Expand Up @@ -126,9 +127,9 @@ function renderIPyWidget(
container: HTMLElement,
logger: (message: string) => void
) {
console.log(`Rendering IPyWidget ${outputId} with model ${model.model_id}`);
logger(`Rendering IPyWidget ${outputId} with model ${model.model_id}`);
if (renderedWidgets.has(outputId)) {
return console.error('already rendering');
return logger('already rendering');
}
const output = document.createElement('div');
output.className = 'cell-output cell-output';
Expand Down Expand Up @@ -241,10 +242,10 @@ let capturedContext: KernelMessagingApi | undefined;
// To ensure we initialize after the other scripts, wait for them.
function attemptInitialize(context?: KernelMessagingApi) {
capturedContext = capturedContext || context;
console.log('Attempt Initialize IpyWidgets kernel.js', context);
logMessage(`Attempt Initialize IpyWidgets kernel.js : ${JSON.stringify(context)}`);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
if ((window as any).vscIPyWidgets) {
console.log('IPyWidget kernel initializing...');
logMessage('IPyWidget kernel initializing...');
initialize(capturedContext);
} else {
setTimeout(attemptInitialize, 100);
Expand Down
2 changes: 0 additions & 2 deletions src/webviews/webview-side/ipywidgets/renderer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ function hookupTestScripts(context: RendererContext<unknown>) {
message: 'Hook not registered'
});
}
console.log(`No Widgetentry point`);
return;
}
if (context.postMessage) {
Expand All @@ -76,7 +75,6 @@ function hookupTestScripts(context: RendererContext<unknown>) {
message: 'Hook registered'
});
}
console.log(`Widgetentry point found`);
anyWindow.widgetEntryPoint.initialize(context);
}
function sendRenderOutputItem(context: RendererContext<unknown>, outputItem: OutputItem, element: HTMLElement) {
Expand Down
9 changes: 0 additions & 9 deletions src/webviews/webview-side/react-common/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,6 @@ export function logMessage(message: string) {
messageLogger(message);
}
}
/**
* Logging in production seems to slow down webview (unnecessarily too chatty)
*/
export function logMessageOnlyOnCI(message: string) {
// TODO: No way to check if CI for webview
if (messageLogger) {
messageLogger(message);
}
}

export function setLogger(logger: (message: string) => void) {
messageLogger = logger;
Expand Down
6 changes: 1 addition & 5 deletions src/webviews/webview-side/react-common/postOffice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Subject } from 'rxjs/Subject';
import { VSCodeEvent } from 'vscode-notebook-renderer/events';
import { WebviewMessage } from '../../../platform/common/application/types';
import { IDisposable } from '../../../platform/common/types';
import { logMessage, logMessageOnlyOnCI } from './logger';
import { logMessage } from './logger';

export interface IVsCodeApi {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down Expand Up @@ -77,7 +77,6 @@ class VsCodeMessageApi implements IMessageApi {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public sendMessage(type: string, payload?: any) {
if (this.vscodeApi) {
logMessageOnlyOnCI(`UI PostOffice Sent ${type}`);
this.vscodeApi.postMessage({ type: type, payload });
} else if (type === 'IPyWidgets_logMessage') {
logMessage(`Logging message ${type}, ${payload}`);
Expand Down Expand Up @@ -224,9 +223,6 @@ export class PostOffice implements IDisposable {
private async handleMessage(msg: WebviewMessage) {
if (this.handlers) {
if (msg) {
if ('type' in msg && typeof msg.type === 'string') {
logMessageOnlyOnCI(`UI PostOffice Received ${msg.type}`);
}
this.subject.next({ type: msg.type, payload: msg.payload });
this.handlers.forEach((h: IMessageHandler | null) => {
if (h) {
Expand Down