From 7b18c1fb8fe931c8bf8dc179a590114b1860761c Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 17 Apr 2020 17:55:41 -0700 Subject: [PATCH 1/3] Add message --- package.nls.json | 4 +- src/client/common/utils/localize.ts | 5 +++ .../ipyWidgetScriptSourceProvider.ts | 41 ++++++++++++++++++- src/client/telemetry/index.ts | 4 ++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/package.nls.json b/package.nls.json index d8c031a6c300..30169e54d0a5 100644 --- a/package.nls.json +++ b/package.nls.json @@ -152,6 +152,7 @@ "Common.ok": "Ok", "Common.install": "Install", "Common.learnMore": "Learn more", + "Common.reportThisIssue": "Report this issue", "OutputChannelNames.languageServer": "Python Language Server", "OutputChannelNames.python": "Python", "OutputChannelNames.pythonTest": "Python Test Log", @@ -471,5 +472,6 @@ "DataScience.loadClassFailedWithNoInternet": "Error loading {0}:{1}. Internet connection required for loading 3rd party widgets.", "DataScience.useCDNForWidgets": "Widgets require us to download supporting files from a 3rd party website. Click [here](https://aka.ms/PVSCIPyWidgets) for more information.", "DataScience.loadThirdPartyWidgetScriptsPostEnabled": "Please restart the Kernel when changing the setting 'python.dataScience.widgetScriptSources'.", - "DataScience.enableCDNForWidgetsSetting": "Widgets require us to download supporting files from a 3rd party website. Click here to enable this or click here for more information. (Error loading {0}:{1})." + "DataScience.enableCDNForWidgetsSetting": "Widgets require us to download supporting files from a 3rd party website. Click here to enable this or click here for more information. (Error loading {0}:{1}).", + "DataScience.widgetScriptNotFoundOnCDNWidgetMightNotWork": "Unable to load a compatible version of the widget '{0}'. Expected behavior may be affected." } diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index ed4c4d03cec1..fd7cfddbcf0b 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -75,6 +75,7 @@ export namespace Common { export const moreInfo = localize('Common.moreInfo', 'More Info'); export const learnMore = localize('Common.learnMore', 'Learn more'); export const and = localize('Common.and', 'and'); + export const reportThisIssue = localize('Common.reportThisIssue', 'Report this issue'); } export namespace AttachProcess { @@ -865,6 +866,10 @@ export namespace DataScience { 'DataScience.enableCDNForWidgetsSetting', "Widgets require us to download supporting files from a 3rd party website. Click here to enable this or click here for more information. (Error loading {0}:{1})." ); + export const widgetScriptNotFoundOnCDNWidgetMightNotWork = localize( + 'DataScience.widgetScriptNotFoundOnCDNWidgetMightNotWork', + "Unable to load a compatible version of the widget '{0}'. Expected behavior may be affected." + ); } export namespace DebugConfigStrings { diff --git a/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts index 054fdd569b04..169ad2e5714b 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts @@ -6,6 +6,7 @@ import { sha256 } from 'hash.js'; import { ConfigurationChangeEvent, ConfigurationTarget } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; +import '../../common/extensions'; import { traceError } from '../../common/logger'; import { IFileSystem } from '../../common/platform/types'; import { @@ -17,6 +18,7 @@ import { } from '../../common/types'; import { createDeferred, Deferred } from '../../common/utils/async'; import { Common, DataScience } from '../../common/utils/localize'; +import { noop } from '../../common/utils/misc'; import { IInterpreterService } from '../../interpreter/contracts'; import { sendTelemetryEvent } from '../../telemetry'; import { Telemetry } from '../constants'; @@ -27,6 +29,7 @@ import { RemoteWidgetScriptSourceProvider } from './remoteWidgetScriptSourceProv import { IWidgetScriptSourceProvider, WidgetScriptSource } from './types'; const GlobalStateKeyToTrackIfUserConfiguredCDNAtLeastOnce = 'IPYWidgetCDNConfigured'; +const GlobalStateKeyToNeverWarnAboutScriptsNotFoundOnCDN = 'IPYWidgetNotFoundOnCDN'; /** * This class decides where to get widget scripts from. @@ -35,6 +38,7 @@ const GlobalStateKeyToTrackIfUserConfiguredCDNAtLeastOnce = 'IPYWidgetCDNConfigu * If user has not configured antying, user will be presented with a prompt. */ export class IPyWidgetScriptSourceProvider implements IWidgetScriptSourceProvider { + private readonly notifiedUserAboutWidgetScriptNotFound = new Set(); private scriptProviders?: IWidgetScriptSourceProvider[]; private configurationPromise?: Deferred; private get configuredScriptSources(): readonly WidgetCDNs[] { @@ -42,6 +46,7 @@ export class IPyWidgetScriptSourceProvider implements IWidgetScriptSourceProvide return settings.datascience.widgetScriptSources; } private readonly userConfiguredCDNAtLeastOnce: IPersistentState; + private readonly neverWarnAboutScriptsNotFoundOnCDN: IPersistentState; constructor( private readonly notebook: INotebook, private readonly localResourceUriConverter: ILocalResourceUriConverter, @@ -57,6 +62,10 @@ export class IPyWidgetScriptSourceProvider implements IWidgetScriptSourceProvide GlobalStateKeyToTrackIfUserConfiguredCDNAtLeastOnce, false ); + this.neverWarnAboutScriptsNotFoundOnCDN = this.stateFactory.createGlobalPersistentState( + GlobalStateKeyToNeverWarnAboutScriptsNotFoundOnCDN, + false + ); } public initialize() { this.workspaceService.onDidChangeConfiguration(this.onSettingsChagned.bind(this)); @@ -94,14 +103,44 @@ export class IPyWidgetScriptSourceProvider implements IWidgetScriptSourceProvide sendTelemetryEvent(Telemetry.HashedIPyWidgetNameUsed, undefined, { hashedName: sha256().update(found.moduleName).digest('hex'), - source: found.source + source: found.source, + cdnSearched: this.configuredScriptSources.length > 0 }); if (!found.scriptUri) { traceError(`Script source for Widget ${moduleName}@${moduleVersion} not found`); } + this.handleWidgetSourceNotFoundOnCDN(found).ignoreErrors(); return found; } + private async handleWidgetSourceNotFoundOnCDN(widgetSource: WidgetScriptSource) { + // if widget exists nothing to do. + if (widgetSource.source === 'cdn' || this.neverWarnAboutScriptsNotFoundOnCDN.value === true) { + return; + } + if ( + this.notifiedUserAboutWidgetScriptNotFound.has(widgetSource.moduleName) || + this.configuredScriptSources.length === 0 + ) { + return; + } + this.notifiedUserAboutWidgetScriptNotFound.add(widgetSource.moduleName); + const selection = await this.appShell.showWarningMessage( + DataScience.widgetScriptNotFoundOnCDNWidgetMightNotWork().format(widgetSource.moduleName), + Common.ok(), + Common.doNotShowAgain(), + Common.reportThisIssue() + ); + switch (selection) { + case Common.doNotShowAgain(): + return this.neverWarnAboutScriptsNotFoundOnCDN.updateValue(true); + case Common.reportThisIssue(): + return this.appShell.openUrl('https://aka.ms/CreatePVSCDataScienceIssue'); + default: + noop(); + } + } + private onSettingsChagned(e: ConfigurationChangeEvent) { if (e.affectsConfiguration('dataScience.widgetScriptSources')) { this.rebuildProviders(); diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index ad86f3f75790..90a9d0420359 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1933,6 +1933,10 @@ export interface IEventNamePropertyMapping { * Where did we find the hashed name (CDN or user environment or remote jupyter). */ source?: 'cdn' | 'local' | 'remote'; + /** + * Whether we searched CDN or not. + */ + cdnSearched: boolean; }; /** * Telemetry event sent with name of a Widget found. From 4e49d3fde569edf650836f6ec4572f55d5b4d82a Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 20 Apr 2020 08:52:25 -0700 Subject: [PATCH 2/3] Add tests --- ...ipyWidgetScriptSourceProvider.unit.test.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts index f6bd79bdc657..dae883947f4d 100644 --- a/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts @@ -272,6 +272,47 @@ suite('Data Science - ipywidget - Widget Script Source Provider', () => { assert.deepEqual(values, { moduleName: 'module1', scriptUri: '1', source: 'cdn' }); assert.isFalse(localOrRemoteSource.calledOnce); assert.isTrue(cdnSource.calledOnce); + verify(appShell.showWarningMessage(anything(), anything(), anything(), anything())).never(); + }); + test('When CDN is turned on and widget script is not found, then display a warning about script not found on CDN', async () => { + settings.datascience.widgetScriptSources = ['jsdelivr.com', 'unpkg.com']; + const localOrRemoteSource = localLaunch + ? sinon.stub(LocalWidgetScriptSourceProvider.prototype, 'getWidgetScriptSource') + : sinon.stub(RemoteWidgetScriptSourceProvider.prototype, 'getWidgetScriptSource'); + const cdnSource = sinon.stub(CDNWidgetScriptSourceProvider.prototype, 'getWidgetScriptSource'); + + localOrRemoteSource.resolves({ moduleName: 'module1' }); + cdnSource.resolves({ moduleName: 'module1' }); + + scriptSourceProvider.initialize(); + let values = await scriptSourceProvider.getWidgetScriptSource('module1', '1'); + + assert.deepEqual(values, { moduleName: 'module1' }); + assert.isTrue(localOrRemoteSource.calledOnce); + assert.isTrue(cdnSource.calledOnce); + verify( + appShell.showWarningMessage( + DataScience.widgetScriptNotFoundOnCDNWidgetMightNotWork().format('module1'), + anything(), + anything(), + anything() + ) + ).once(); + + // Ensure message is not displayed more than once. + values = await scriptSourceProvider.getWidgetScriptSource('module1', '1'); + + assert.deepEqual(values, { moduleName: 'module1' }); + assert.isTrue(localOrRemoteSource.calledTwice); + assert.isTrue(cdnSource.calledTwice); + verify( + appShell.showWarningMessage( + DataScience.widgetScriptNotFoundOnCDNWidgetMightNotWork().format('module1'), + anything(), + anything(), + anything() + ) + ).once(); }); }); }); From 83419665df95c9cb8c7af4728de98469d21638db Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 20 Apr 2020 08:56:29 -0700 Subject: [PATCH 3/3] Fix typo --- .../datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts index 169ad2e5714b..9896d7a7b9bf 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts @@ -142,7 +142,7 @@ export class IPyWidgetScriptSourceProvider implements IWidgetScriptSourceProvide } private onSettingsChagned(e: ConfigurationChangeEvent) { - if (e.affectsConfiguration('dataScience.widgetScriptSources')) { + if (e.affectsConfiguration('python.dataScience.widgetScriptSources')) { this.rebuildProviders(); } }