-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support custom widgets #47
Merged
captainsafia
merged 11 commits into
nteract:master
from
vivek1729:vipradha/customWidgets
Dec 3, 2020
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
947756f
vp - Add support for loading 3rd party widgets from CDN
vivek1729 a6b63c2
vp - Add extensibility point for providing custom loader for 3rd part…
vivek1729 2ea82ee
vp - Fix typos, minor formating, URL sanitization
vivek1729 aadbe7c
vp - Rename files to match nteract pattern
vivek1729 af29994
vp - Address review comments to update tests
vivek1729 658b54f
vp - Simplify requireLoader to return a promise, address more review …
vivek1729 8dc6a5e
vp - Fix documentation
vivek1729 4e8eba6
vp - Minor formatting fix
vivek1729 f2ce589
vp - Update README
vivek1729 8e89ce8
Update packages/jupyter-widgets/README.md
vivek1729 e693ef4
vp - Add link to requireJS docs for error message
vivek1729 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,88 @@ | ||
import { IntSliderView } from "@jupyter-widgets/controls"; | ||
import { Map } from "immutable"; | ||
|
||
import { ManagerActions } from "../../src/manager/index"; | ||
import { WidgetManager } from "../../src/manager/widget-manager"; | ||
import * as customWidgetLoader from "../../src/manager/widget-loader"; | ||
|
||
// A mock valid module representing a custom widget | ||
const mockFooModule = { | ||
"foo" : "bar" | ||
}; | ||
// Mock implementation of the core require API | ||
const mockRequireJS = jest.fn((modules, ready, errCB) => ready(mockFooModule)); | ||
(window as any).requirejs = mockRequireJS; | ||
(window as any).requirejs.config = jest.fn(); | ||
|
||
// Manager actions passed as the third arg when instantiating the WidgetManager class | ||
const mockManagerActions: ManagerActions["actions"] = { | ||
appendOutput: jest.fn(), | ||
clearOutput: jest.fn(), | ||
updateCellStatus: jest.fn(), | ||
promptInputRequest: jest.fn() | ||
}; | ||
|
||
// Default modelById stub | ||
const mockModelById = (id: string) => undefined; | ||
|
||
describe("WidgetManager", () => { | ||
describe("loadClass", () => { | ||
beforeAll(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it("returns a class if it exists", () => { | ||
const modelById = (id: string) => undefined; | ||
const manager = new WidgetManager(null, modelById); | ||
const manager = new WidgetManager(null, mockModelById, mockManagerActions); | ||
const view = manager.loadClass( | ||
"IntSliderView", | ||
"@jupyter-widgets/controls", | ||
"1.5.0" | ||
); | ||
expect(view).not.toBe(null); | ||
}); | ||
|
||
it("Returns a valid module class successfully from CDN for custom widgets", () => { | ||
const manager = new WidgetManager(null, mockModelById, mockManagerActions); | ||
const requireLoaderSpy = jest.spyOn(customWidgetLoader, "requireLoader"); | ||
|
||
return manager.loadClass( | ||
"foo", | ||
"fooModule", | ||
"1.1.0" | ||
).then(view => { | ||
expect(requireLoaderSpy).toHaveBeenCalledTimes(1); | ||
// Get the second arg to Monaco.editor.create call | ||
const mockLoaderArgs = requireLoaderSpy.mock.calls[0]; | ||
expect(mockLoaderArgs).not.toBe(null); | ||
expect(mockLoaderArgs.length).toBe(2); | ||
expect(mockLoaderArgs[0]).toBe("fooModule"); | ||
expect(mockLoaderArgs[1]).toBe("1.1.0"); | ||
expect(view).not.toBe(null); | ||
expect(view).toBe(mockFooModule["foo"]); | ||
}); | ||
}); | ||
|
||
it("Returns an error if the class does not exist on the module", () => { | ||
const manager = new WidgetManager(null, mockModelById, mockManagerActions); | ||
const requireLoaderSpy = jest.spyOn(customWidgetLoader, "requireLoader"); | ||
|
||
return manager.loadClass( | ||
"INVALID_CLASS", | ||
"fooModule", | ||
"1.1.0" | ||
).catch(error => { | ||
expect(requireLoaderSpy).toHaveBeenCalledTimes(1); | ||
expect(error).toBe("Class INVALID_CLASS not found in module [email protected]"); | ||
}); | ||
}); | ||
}); | ||
|
||
describe("create_view", () => { | ||
it("returns a widget mounted on the provided element", async () => { | ||
const modelById = (id: string) => undefined; | ||
const manager = new WidgetManager(null, modelById); | ||
const manager = new WidgetManager(null, mockModelById, mockManagerActions); | ||
const model = { | ||
_dom_classes: [], | ||
_model_module: "@jupyter-widgets/controls", | ||
|
@@ -99,7 +162,7 @@ describe("WidgetManager", () => { | |
const model = id === "layout_id" ? layoutModel : styleModel; | ||
return Promise.resolve(Map({ state: Map(model) })); | ||
}; | ||
const manager = new WidgetManager(null, modelById); | ||
const manager = new WidgetManager(null, modelById, mockManagerActions); | ||
const widget = await manager.new_widget_from_state_and_id( | ||
model, | ||
"test_model_id" | ||
|
@@ -119,11 +182,10 @@ describe("WidgetManager", () => { | |
}); | ||
}); | ||
it("can update class properties via method", () => { | ||
const modelById = (id: string) => undefined; | ||
const manager = new WidgetManager(null, modelById); | ||
const manager = new WidgetManager(null, mockModelById, mockManagerActions); | ||
expect(manager.kernel).toBeNull(); | ||
const newKernel = { channels: { next: jest.fn() } }; | ||
manager.update(newKernel, modelById, {}); | ||
manager.update(newKernel, mockModelById, mockManagerActions); | ||
expect(manager.kernel).toBe(newKernel); | ||
}); | ||
}); |
63 changes: 63 additions & 0 deletions
63
packages/jupyter-widgets/__tests__/manager/widget-loader.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
import { requireLoader } from "../../src/manager/widget-loader"; | ||
|
||
// A mock valid module representing a custom widget | ||
const mockModule = { | ||
"foo" : "bar" | ||
}; | ||
// Info representing an invalid module for testing the failure case | ||
const invalidModule = { | ||
name: "invalid_module", | ||
version: "1.0", | ||
url: "https://unpkg.com/[email protected]/dist/index.js" | ||
}; | ||
// Mock implementation of the core require API | ||
const mockRequireJS = jest.fn((modules, ready, errCB) => { | ||
if(modules.length > 0 && modules[0] === invalidModule.url){ | ||
errCB(new Error("Whoops!")); | ||
} | ||
else { | ||
ready(mockModule); | ||
} | ||
}); | ||
|
||
// Callback binding | ||
(window as any).requirejs = mockRequireJS; | ||
(window as any).requirejs.config = jest.fn(); | ||
|
||
describe("requireLoader", () => { | ||
beforeAll(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
it("Returns a module if linked to a valid CDN URL", () => { | ||
return requireLoader("foo", "1.0.0").then(mod => { | ||
expect(mockRequireJS).toHaveBeenCalledTimes(1); | ||
const moduleURLs = mockRequireJS.mock.calls[0][0]; | ||
expect(moduleURLs).not.toBe(null); | ||
expect(moduleURLs.length).toBe(1); | ||
expect(moduleURLs[0]).toBe("https://unpkg.com/[email protected]/dist/index.js"); | ||
expect(mod).toEqual(mockModule); | ||
}); | ||
}); | ||
|
||
it("Returns a module even when module version is missing", () => { | ||
return requireLoader("foo", undefined).then(mod => { | ||
expect(mockRequireJS).toHaveBeenCalledTimes(1); | ||
const moduleURLs = mockRequireJS.mock.calls[0][0]; | ||
expect(moduleURLs).not.toBe(null); | ||
expect(moduleURLs.length).toBe(1); | ||
expect(moduleURLs[0]).toBe("https://unpkg.com/foo/dist/index.js"); | ||
expect(mod).toEqual(mockModule); | ||
}); | ||
}); | ||
|
||
it("Calls the error callback if an error is encountered during the module loading", () => { | ||
const {name, version} = invalidModule; | ||
return requireLoader(name, version).catch((error: Error) => { | ||
expect(mockRequireJS).toHaveBeenCalledTimes(1); | ||
expect(error.message).toBe("Whoops!"); | ||
}); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
/** | ||
* Several functions in this file are based off the html-manager in jupyter-widgets project - | ||
* https://github.com/jupyter-widgets/ipywidgets/blob/master/packages/html-manager/src/libembed-amd.ts | ||
*/ | ||
|
||
import * as base from "@jupyter-widgets/base"; | ||
import * as controls from "@jupyter-widgets/controls"; | ||
|
||
const requireJSMissingErrorMessage = "Requirejs is needed, please ensure it is loaded on the page. Docs - https://requirejs.org/docs/api.html"; | ||
let cdn = "https://unpkg.com"; | ||
|
||
/** | ||
* Constructs a well formed module URL for requireJS | ||
* mapping the modulename and version from the base CDN URL | ||
* @param moduleName Name of the module corresponding to the widget package | ||
* @param moduleVersion Module version returned from kernel | ||
*/ | ||
function moduleNameToCDNUrl(moduleName: string, moduleVersion: string): string { | ||
let packageName = moduleName; | ||
let fileName = "index.js"; // default filename | ||
// if a '/' is present, like 'foo/bar', packageName is changed to 'foo', and path to 'bar' | ||
// We first find the first '/' | ||
let index = moduleName.indexOf('/'); | ||
if (index !== -1 && moduleName[0] === '@') { | ||
// if we have a namespace, it's a different story | ||
// @foo/bar/baz should translate to @foo/bar and baz | ||
// so we find the 2nd '/' | ||
index = moduleName.indexOf('/', index + 1); | ||
} | ||
if (index !== -1) { | ||
fileName = moduleName.substr(index + 1); | ||
packageName = moduleName.substr(0, index); | ||
} | ||
const moduleNameString = moduleVersion ? `${packageName}@${moduleVersion}` : packageName; | ||
return `${cdn}/${moduleNameString}/dist/${fileName}`; | ||
} | ||
|
||
/** | ||
* Load a package using requirejs and return a promise | ||
* | ||
* @param pkg Package name or names to load | ||
*/ | ||
function requirePromise(pkg: string | string[]): Promise<any> { | ||
return new Promise((resolve, reject) => { | ||
const require = (window as any).requirejs; | ||
if (require === undefined) { | ||
reject(requireJSMissingErrorMessage); | ||
} | ||
else { | ||
// tslint:disable-next-line: non-literal-require | ||
require(pkg, resolve, reject); | ||
} | ||
}); | ||
}; | ||
|
||
/** | ||
* Initialize dependencies that need to be preconfigured for requireJS module loading | ||
* Here, we define the jupyter-base, controls package that most 3rd party widgets depend on | ||
*/ | ||
export function initRequireDeps(){ | ||
vivek1729 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Export the following for `requirejs`. | ||
// tslint:disable-next-line: no-any no-function-expression no-empty | ||
const define = (window as any).define || function () {}; | ||
define("@jupyter-widgets/controls", () => controls); | ||
define("@jupyter-widgets/base", () => base); | ||
} | ||
|
||
/** | ||
* Overrides the default CDN base URL by querying the DOM for script tags | ||
* By default, the CDN service used is unpkg.com. However, this default can be | ||
* overriden by specifying another URL via the HTML attribute | ||
* "data-jupyter-widgets-cdn" on a script tag of the page. | ||
*/ | ||
export function overrideCDNBaseURL(){ | ||
// find the data-cdn for any script tag | ||
const scripts = document.getElementsByTagName("script"); | ||
Array.prototype.forEach.call(scripts, (script: HTMLScriptElement) => { | ||
cdn = script.getAttribute("data-jupyter-widgets-cdn") || cdn; | ||
}); | ||
// Remove Single/consecutive trailing slashes from the URL to sanitize it | ||
cdn = cdn.replace(/\/+$/, ""); | ||
} | ||
|
||
/** | ||
* Load an amd module from a specified CDN | ||
* | ||
* @param moduleName The name of the module to load. | ||
* @param moduleVersion The semver range for the module, if loaded from a CDN. | ||
* | ||
* By default, the CDN service used is unpkg.com. However, this default can be | ||
* overriden by specifying another URL via the HTML attribute | ||
* "data-jupyter-widgets-cdn" on a script tag of the page. | ||
* | ||
* The semver range is only used with the CDN. | ||
*/ | ||
export function requireLoader(moduleName: string, moduleVersion: string): Promise<any> { | ||
const require = (window as any).requirejs; | ||
if (require === undefined) { | ||
vivek1729 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return Promise.reject(new Error(requireJSMissingErrorMessage)); | ||
} | ||
else { | ||
const conf: { paths: { [key: string]: string } } = { paths: {} }; | ||
const moduleCDN = moduleNameToCDNUrl(moduleName, moduleVersion); | ||
conf.paths[moduleName] = moduleCDN; | ||
require.config(conf); | ||
return requirePromise([moduleCDN]); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@rgbkrk Thoughts on this model for passing custom properties to the JavaScript assets?
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.
Interesting -- this makes it so a custom frontend can specify where custom widgets are allowed to be loaded from?
Probably worth noting that any output can add a tag to say to load other assets, so that's another vector for attack (though it means you need some HTML output already emitted in the document to use it).
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.
Correct. This allows you to set the CDN to use when loading widgets. The HTML widget manager does something similar (ref).
Good point. The logic currently loads the the attribute from any script tag. I believe the way the for-loop is setup will cause it to favor the most recently inserted script tag which might be a problem.
@vivek1729 Thoughts?
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.
That's a good point @rgbkrk. @captainsafia rightly pointed out that I followed the pattern that the HTML widget manager to override the default CDN URL. The logic of reading and looping over
script
tags is also based off the HTML Widget manager (ref).Trying to understand how favoring the most recent added script tag might be a problem. Ideally, we'd hope that there's only 1 script tag that specifies the
data-jupyter-widgets-cdn
tag. Looping over script tags potentially opens up the possibility of script tags being added dynamically which can lead to the cdn URL being over-written. However, we should also note that the CDN url is set when theWidgetManager
is getting constructed. Since theWidgetManager
is a singleton, this would only happen once and it makes sense because we don't really want to have the base CDN URL change very often. It's more like a one-time setup configuration.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.
The
WidgetManager
gets instantiated once when the first widget is rendered onto the page. It's possible for a nefarious to render a HTML output (or inject HTML into the page in some other way) that renders a script tag with a malicious CDN onto the page then render a second output containing the ipywidget.@jasongrout Do you have any thoughts on this since I believe you implemented the original code in the HTML manager?
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.
@captainsafia, thanks for the additional details. How do you suggest we address this concern?
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.
For now, I'm thinking that we:
If that sounds good to you we can move forward with it.
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.
@captainsafia , this sounds like a good plan to me.
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'll proceed with the PR when you get a chance to do a final review and approve.