Support using a CDN for widgets #10076

rchiodo · 2022-05-19T23:07:29Z

Most widgets will work after this change but not all. Some require js to load from the jupyter server and that requires some changes in VS code to accomplish #10060

rchiodo · 2022-05-19T23:11:43Z

src/kernels/ipywidgets-message-coordination/scriptUriConverter.ts

@@ -19,11 +19,11 @@ export class ScriptUriConverter implements ILocalResourceUriConverter {
        return this.requestUriEmitter.event;
    }
    public get rootScriptFolder(): Uri {
-        return Uri.file(this._rootScriptFolder);
+        return this._rootScriptFolder;


This file isn't node specific anymore, but doesn't actually work in the web because we can't write to the extensionUri location. It isn't used for web right now but once #10060 is implemented, it can be used to support that scenario.

That's why I left this being usable for web too.

rchiodo · 2022-05-19T23:12:51Z

src/notebooks/controllers/vscodeNotebookController.ts

@@ -165,7 +167,8 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
            viewType,
            label,
            this.handleExecution.bind(this),
-            this.getRendererScripts()
+            this.getRendererScripts(),
+            [this.scriptConverter.rootScriptFolder]


Whoops. This is the change needed for @mjbvz's new API (which hasn't gone through yet)

I'll just comment this out with a note saying this is how we'd use it.

rchiodo · 2022-05-19T23:18:16Z

src/platform/common/types.ts

@@ -192,29 +192,9 @@ export type DownloadOptions = {
    extension: 'tmp' | string;
 };

-export const IFileDownloader = Symbol('IFileDownloader');


IFileDownloader is not used. Leftovers from the python extension.

rchiodo · 2022-05-19T23:20:11Z

package.json

@@ -2183,8 +2183,6 @@
        "redux": "^4.0.4",
        "redux-logger": "^3.0.6",
        "reflect-metadata": "^0.1.12",
-        "request": "^2.87.0",


Request has been deprecated, switched to 'cross-fetch' instead.

codecov-commenter · 2022-05-19T23:50:46Z

Codecov Report

Merging #10076 (5efd808) into main (7aa2b19) will decrease coverage by 0%.
The diff coverage is 80%.

❗ Current head 5efd808 differs from pull request most recent head 286f375. Consider uploading reports for the commit 286f375 to get more accurate results

@@          Coverage Diff           @@
##            main   #10076   +/-   ##
======================================
- Coverage     64%      64%   -1%     
======================================
  Files        216      214    -2     
  Lines      10060     9989   -71     
  Branches    1613     1605    -8     
======================================
- Hits        6509     6442   -67     
+ Misses      3028     3023    -5     
- Partials     523      524    +1

Impacted Files	Coverage Δ
src/platform/common/application/types.ts	`100% <ø> (ø)`
src/platform/common/platform/errors.ts	`88% <ø> (ø)`
src/platform/common/platform/types.node.ts	`100% <ø> (ø)`
src/platform/common/platform/types.ts	`100% <ø> (ø)`
src/platform/common/process/condaService.node.ts	`71% <0%> (ø)`
src/platform/common/types.ts	`100% <ø> (ø)`
src/platform/common/net/httpClient.ts	`64% <64%> (ø)`
src/platform/common/platform/fileSystem.ts	`77% <80%> (+5%)`	⬆️
src/platform/common/application/notebook.ts	`91% <100%> (ø)`
src/platform/common/platform/fileSystem.node.ts	`85% <100%> (+5%)`	⬆️
... and 8 more

rchiodo · 2022-05-19T23:54:21Z

src/platform/common/net/httpClient.ts

+        return fetch.fetch(uri, this.requestOptions);
+    }
+
+    public async getJSON<T>(uri: string, strict: boolean = true): Promise<T> {


Going to remove these. Not used anywhere.

I removed the unit test for this class well because it's just a light wrapper around fetch. I don't think it needs a unit test

amunger · 2022-05-20T17:07:13Z

src/platform/common/net/httpClient.ts

+@injectable()
+export class HttpClient implements IHttpClient {
+    public readonly requestOptions: RequestInit;
+    constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {


why inject the entire service container instead of the actual dependencies?

It was doing that before, I didn't notice. I'll change it if there's another update.

amunger · 2022-05-20T17:09:43Z

src/kernels/ipywidgets-message-coordination/cdnWidgetScriptSourceProvider.base.ts

@@ -0,0 +1,99 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


is '.base' needed in the filename?

No. I just used it to distinguish that this is a base class for the .node and .web files. I've done this in other spots.

amunger · 2022-05-20T17:23:38Z

src/platform/common/platform/fileSystem.ts

@@ -101,4 +109,64 @@ export class FileSystem implements IFileSystem {
        const data = typeof text === 'string' ? Buffer.from(text) : text;
        return this.vscfs.writeFile(uri, data);
    }
+
+    async createTemporaryFile(options: { fileExtension?: string; prefix?: string }): Promise<TemporaryFileUri> {
+        // In non node situations, temporary files are created in the globalStorageUri location (extension specific)


This seems to be in node context as well since it's in a common file. Does globalStorageUri mean different things in those contexts, or will this only be called in web?

No it works for both. It's intended to work for both (using it in the tests now). globalStorageUri is just a location on disk for desktop, and it's an in memory location for web.

I should change the comment though.

rchiodo added 8 commits May 17, 2022 16:43

Refactor CDN and download stuff

c0ec7d9

Remove client check for remote. It never works anyway

81e5715

Addition to proposed API for local resource roots

7e78676

Workaround inability to download and upload CDN files

020ec73

Merge remote-tracking branch 'origin/main' into rchiodo/ipywidgets_web_2

a66ff8a

Make widget tests run in web too

ecc2703

Skip some output unless CI and capture failures for node

70b0840

add news entry

36d918c

rchiodo requested a review from a team as a code owner May 19, 2022 23:07

rchiodo commented May 19, 2022

View reviewed changes

rchiodo added 2 commits May 19, 2022 16:14

Remove unavailable proposed api usage

29a94fe

Comment in the actual API location

7569d97

rchiodo commented May 19, 2022

View reviewed changes

rchiodo added 2 commits May 19, 2022 16:33

Hopefully fix build and get rid of pointless unit test

68a7e0e

Preserve kernel metadata from template

3b8ec99

rchiodo commented May 19, 2022

View reviewed changes

rchiodo added 2 commits May 19, 2022 16:55

Remove unused stuff

6cf04de

I'm an idiot

5efd808

amunger reviewed May 20, 2022

View reviewed changes

amunger approved these changes May 20, 2022

View reviewed changes

IanMatthewHuff approved these changes May 20, 2022

View reviewed changes

Review feedback

286f375

rchiodo merged commit c214534 into main May 20, 2022

rchiodo deleted the rchiodo/ipywidgets_web_2 branch May 20, 2022 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using a CDN for widgets #10076

Support using a CDN for widgets #10076

rchiodo commented May 19, 2022

rchiodo May 19, 2022

rchiodo May 19, 2022

rchiodo May 19, 2022

rchiodo May 19, 2022

codecov-commenter commented May 19, 2022 •

edited

Loading

rchiodo May 19, 2022

rchiodo May 19, 2022

amunger May 20, 2022

rchiodo May 20, 2022

amunger May 20, 2022

rchiodo May 20, 2022

amunger May 20, 2022

rchiodo May 20, 2022

		@@ -0,0 +1,99 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

Support using a CDN for widgets #10076

Support using a CDN for widgets #10076

Conversation

rchiodo commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2022 •

edited

Loading