PTV-1905 - add service widget support #3421
Conversation
|
Hi @briehl , this is the WIP PR for adding service widgets to the Narrative. |
|
I think using preact is fine, spending time reworking into jquery isn't gonna help that much. I'm still picking through reading this, as there's a lot to review. But we can get it up on CI anyway to keep you unblocked. |
|
Thanks @briehl! The service widgets team will be elated! I'll catch up the branch with develop later before we deploy it on CI, and get those merge conflicts resolved. Also, I've been meaning to confirm this - it looks like each PR includes Of course, I'm familiar with this practice. That is how old kbase-ui plugins worked, as there was no "build" for them on the github side. Not a big deal, but I know the Narrative has some other files that get changed during development that should be kept out of commits, like the config and narrative version files. |
c'mon peeps, lets start the (p)React train!
or at least try another idea.
…le wrapper [PTV-1905]
|
@ialarmedalien I think we just updated to node v20.11.1 (the current LTS) on the image. I'm not sure about which npm version is required, but I'm guessing the recent LTS should work best. |
|
@briehl is there a new base image? I just opened a terminal into the 7.1.0 image and see: (venv) erikpearson@Eriks-MacBook-Pro narrative % docker run -it ghcr.io/kbase/narrative-base-image:7.1.0 bash
root@970e2b5fe08f:/# node --version
v16.17.0
root@970e2b5fe08f:/# npm --version
8.15.0
root@970e2b5fe08f:/# I was about to mention that the npm install is hanging in a the base image container ... but it just takes about 10-15 minutes to for msw (or rxjs subdependency). I had noticed that the image build appeared to be hanging, so opened a bash shell into a base image container, did a fresh install and didn't get much more information than that. npm install works fine on 8.19.4 macOS, but in the container has this hanging issue (which does eventually resolve.) I tried with the original package and lock file, and with one updated with prop types, which forces a lock file rebuild. |
|
Actually, it was hanging on nodemon, but that is a transitive dependency of the catalog plugin, which I temporarily removed since it is for just one small yaml file, and then it began to hang on msw. I figured nodemon, being os-dependent, was the culprit, but not. It still could be an os-dependent binary component, though, as it does not hang on macos. |
|
I forgot to mention that the extra detail inside the container is that "reify:rxjs" is revealed: npm timing build:run:postinstall:node_modules/nodemon Completed in 142ms
(##################) ⠸ reify:rxjs: timing build:run:postinstall:node_modules/nodemon Completed in 142msand finally npm timing npm Completed in 670141msmany many people have reported this for node 16 and whatever npm shipped with it, but it really isn't worth pursuing if we're moving on to node 20 and npm 10. |
|
@briehl So I went down this rabbit hole, and managed to get preact debug and devtool support configured for requirejs, preact_compat configured and working, and prop-types installed configured as well. Also preact devtools in the browser (FF). However, so far cannot get preact debug mode enabled, which is required for propTypes to be activated. I added a "gallery" page for the ErrorAlert component, and can get everything wired up so that it renders correctly, but just cannot get propTypes to do anything. There are other features of debug and devtool support, like detecting incorrect usage of a component. All of these are supported through detecting the conditions and issuing warnings to the console. The preact devtool extension works and recognizes preact, so it is at least partially working. One of the tricks seems to be to get the correct load order of debug and preact, and to satisfy the conditions for debug mode to activate itself, but so far no dice. It is not very well (extensively) documented, and certainly not for UMD/AMD usage. I'm going to stop this for now, but may contact the preact team to see if this use case is supported and, if so, how. |
|
@briehl I can check in the changes with the seemingly correct dependencies, to preserve that work. |
and rebuilt package-lock.json [PTV-2905]
|
@briehl I've asked in the preact slack about getting prop types working with the requirejs/amd scenario. So ... I'm not sure it is worth adding the dependencies. Maybe if I get a useful response we can add it. |
also add a gallery page to demostrate it
aids in iteration with dependencies
|
@briehl Actually, I just got it working. The docs say that it should issue a console warning on a validation fail, so I was filtering the console on warnings, but then I saw someone say that it is actually issued as console.error, but with the message prefix of "Warning: ..." I would have surely have eventually noticed the errors... E.g. Failed prop type: Invalid prop At least the error message is no longer prefixed with "Warning:" ! |
|
Now that it is working, I'll go back and add proptypes to all of the preact components. |
|
@eapearson I'm still somewhat wrestling with dependencies and builds between the narrative-base-image and narrative repos. Hopefully those will get ironed out soon - right now there's some change that's affecting unit tests in an async/hard to repeat way. Then we can see if (unit, at least) tests will pass and try to finalize a review here. |
|
I'll keep poking at this every couple of days. Got through one batch of prop-types, will do another round next week. |
…es references [PTV-1905]
should circle back and take care of the remaining TODOs.
|
Had another look at the object spread issue, since it is the source of most of the sonar issues. It ends up that esprima, which has been poorly maintained and has a longstanding bug wrt object spread, is used by two of our dependencies - terser and requirejs's r.js. Terser has a solution, which is to use acorn for parsing. Requirejs, however, has not really been touched in years, so is very unlikely to ever be switched away from esprima, which itself is unlikely to ever be fixed. Interestingly, eslint used to use esprima, but switched away to to espree/acorn. |
|
@briehl prop-type support has been added to all the preact components. Tests to cover then can be added at some point - they will just have to test that the appropriate warning is issued to the console. For now, I added gallery pages for some of the affected components, so that at least one can poke at the components with props that violate the type specs, and manually inspect the warnings. |
|
There was a problem hiding this comment.
A long time coming, but I think - technically - this is great. I still kinda have issues with the overall design of the project and implications it'll have, but the code here in this PR isn't a reflection on that, and is fine. I have a few comments that I've built up over time spent reviewing, but I think they can be addressed in a future PR.
| Two new external libraries are added; `preact` and `htm` have been added to support a | ||
| more familiar (dare we say "modern"?) style of component architecture. | ||
|
|
||
| > This decision can be reversed, and we can rewrite the components in jquery, but the |
| } | ||
| renderContent() { | ||
| if (this.props.render) { | ||
| return this.props.render(); |
There was a problem hiding this comment.
I might be misreading this, but will this cause some strange looping if the render prop is the same as the render function?
What's the render prop expected to be here?
There was a problem hiding this comment.
I may have made this comment before you started adding PropTypes
| require(['jquery', 'narrativeConfig', 'narrativeLogin'], ($, Config, Login) => { | ||
| console.log('Loading KBase Narrative setup routine.'); | ||
|
|
||
| require(['jquery', 'narrativeConfig', 'narrativeLogin', 'preact_debug'], ($, Config, Login) => { |
There was a problem hiding this comment.
How does this differ from preact? Is it preferred over that?
| .gallery h2 { | ||
| } | ||
|
|
||
|
|
||
| .gallery h3 { | ||
| } | ||
|
|
| * @param {string} featureName The name of the given feature. | ||
| * @returns {boolean} Whether the given feature is enabled | ||
| */ | ||
| function isFeatureEnabled(featureName) { |
There was a problem hiding this comment.
This should figure out all enabled features in the factory constructor, and only do it once per session. Right now, if I make a cell, then delete the slug with the feature, and make another cell, they'll behave differently, which is kinda weird.
Also, it sorta hammers the browser to look up this stuff every time a new Runtime is needed.
| console.warn('Already the first cell!'); | ||
| return; | ||
| } | ||
| Jupyter.notebook.move_cell_up(); |
There was a problem hiding this comment.
Why use move_cell_up / down here and move_selection_up / down above?
move_cell_up throws a deprecation warning anyway, and with the other changes, might as well change to move_selection_* here.
| @@ -243,6 +414,24 @@ define([ | |||
| } | |||
| } | |||
|
|
|||
| const extraIcon = (() => { | |||
| 'preactComponents/RotatedTable', | ||
|
|
||
| // for effect | ||
| 'css!./AddServiceWidgetDataViewer.css', |
There was a problem hiding this comment.
Is this needed? Shouldn't it just be part of the CSS stack?
| const { Component } = preactCompat; | ||
| const html = htm.bind(h); | ||
|
|
||
| class ErrorAlert extends Component { |
There was a problem hiding this comment.
It would be ideal to go with functional components to be inline with Europa. I get that this would be a lot of changes, so it doesn't have to be done here. I also foresee having to do a lot of work to migrate to a new Narrative codebase (in pure React / TS) in the not-too-distant future, so maybe don't worry about it now.
There was a problem hiding this comment.
Noting that any major narrative codebase changes will be to support the new versions of Jupyter Notebook / Jupyterlab
| delete constantParams.service_module_name; | ||
| delete constantParams.widget_name; | ||
|
|
||
| const params = Object.assign({}, constantParams, { ref }); |
There was a problem hiding this comment.
Why not just set constantParams[ref] = ref, and use constantParams below?
Description of PR purpose/changes
Changes should not affect anything other than service widgets. Some changes cut across the codebase, but by far most changes only affect the new
nbextensions/serviceWidgetcell.Below is an extensive write-up of the changes:
Changes to Enable Service Widgets in the Narrative
This document summarizes changes required to the Narrative to enable the usage of service widgets.
Summary of Changes
nbextensions/serviceWidget,src/biokbase/cells/service_widget.py,kbase-extension/scss/partials/_serviceWidgetCell.scss,pythonInterop.jskbase-extension/static/kbase/js/util/cellSupport/hasCellBase.jsandCellManager.js, moving sharable cell code out of the specific implementationcellUtils.jsto make life a bit easier.preactandhtmdependencies inpackage.json,narrative_paths.jskbase-extension/static/kbase/js/preactComponents-ErrorAlert,Loading, etc.nbextensions/serviceWidgetCell/util/SendCannelandnbextensions/serviceWidgetCell/util/ReceiveChannelto provide for serviceWidgetCell <-> serviceWidget comm through the window"message"event, aka,postMessage().appCellWidget.jskbaseNarrativeWorkspace.jsandnarrativeViewers.js.AddServiceWidget,AddServiceWidgetDataclasses, which render bits forbootstrapDialog.jsbootstrapDialogto add a size option, and added support the'show.bs.modal'eventkbaseNarrativeto add developer menu dialogs and developer detectionnarrative_header.htmlDetails
Wherein we shall dive deeper into the following areas of implementation:
Implement service widget cell
The service widget cell is implemented as a notebook extension in the directory
nbextensions/serviceWidgetCell.Python and Javascript injection
In addition to the files in this directory, python injection support is required for a new cell type.
pythonInterop.pygains a new function,buildServiceWidgetRunner, to generate the Python snippet for the cell. This snippet relies upon the implementation in the new functionrender_appin a new modulebiokbase.cells.service_widget. This function,in turn, generates the Javascript to be injected into the code cell output area. Finally, the injected Javascript invokes theRootwidget innbextensions/serviceWidgetCell/widgets/Root.js, from which the rest of the components are rendered."cell" implementation
It is worth noting that the cell implementation is divided into two main parts - the notebook cell implementation, which knows how to control the cell itself, and the widget implementation.
The top level of the cell's Javascript implementation is in in
nbextensions/serviceWidgetCell/ServiceWidgetCell.jsandutils/cellSupport/CellManager.js.ServiceWidgetCellis a subclass ofCellBaseinutil/cellSupport/CellBase.js, which provides the bulk of the logic.The "manager" is responsible for integration with the notebook extension mechanism and notebook events. The "cell" is responsible for per-cell KBase behavior as well as specific behavior for the service widget cell.
"widget" implementation
The cell ui, what we think of as the "widget", begins life in the
Rootwidget innbextensions/serviceWidgetCell/widgets/Root.js.TheRootwidget is primarily responsible for bridging between the Narrative and the service widget implementation. It prepares props for and then insertscomponents/Main.js, which is implemented inpreactinhtmand provides a react componentMain.As mentioned above, the cell's widget starts with
nbextensions/serviceWidgetCell/widgets/Root.js. This is simply reponsible for inserting the widget into the DOM and rendering the top level react widget.This top level react component,
nbextensions/serviceWidgetCell/components/Main.js, contains most of the logic to implement the service widget, including communication with the widget (via postMessage), preparation of widget parameters, and insertion of the component which implements the iframe.Finally a component implements the specially-formed iframe in
nbextensions/serviceWidgetCell/components/IFrame.js. This is essentially a simple wrapper which, given a set of props, knows how to create the appropriate iframe markup.Communication between Narrative and embedded service widget app
A service widget cell supports communication with a service widget app embedded in an iframe. To facilitate this, a pair of classes,
SendChannelinnbextensions/serviceWidgetCell/SendChannel.jsandReceiveChannelinnbextensions/serviceWidgetCell/ReceiveChannel.jsare utilized. These are based on the same class I've used for years in kbase-ui to facilitate communication with the plugins using "window messages" via the postMessage api. However, unlike the kkbase-ui plugin implementation, this requires two channels (using the same channel id). This is because the service widget may be running on a different host than the Narrative, and browser security policy prevents Javascript in one window from listening for messages on another window if their origins differ. This will be true in prod.So, the comunication is split into a sender and receiver in order to be able to handle cross-origin communication.
The Narrative <-> Service Widget Cell communication is required to implement the invocation of service widgets, as well as additional features.
Additional features include:
More advanced uses include:
Miscellaneous Support Javascript
With
preactavailable, a few general purpose components were added -Loading,ErrorAlert, andRotatedTable.Basically, any repeated code was moved into a separate component.
The
Loadingcomponent shows a loaindg spinner with an optional message. TheErrorAlertshows a simple error message in an error alert, andRotatedTableimplements a simple two-column table, in which the first column is the header, and the second the value.In summary, all files added or changed to implement it
kbase-extensionstatickbasejscommonpythonInterop.jsutilcellSupportCellBase.jsCellManager.jspreactComponentsErrorAlert.cssErrorAlert.jsLoading.jsLoading.styles.jsnbextensionserviceWidgetCellcomponentsIFrame.cssIFrame.jsMain.cssMain.jsutilReceiveChannel.jsSendChannel.jswidgetsRoot.cssRoot.jsMaincomponent with parameters passed from the cell.constants.jsmain.jsServiceWidgetCellManager.jsreadme.mdServiceWidgetCell.cssServiceWidgetCell.jsService Widget as Data Object Viewer
Enabling service widgets to serve as an object viewer requires changes to the existing Javascript module
widgets/narrative_core/kbaseNarrativeWorkspace.js,fixes towidgets/narrative_core/narrativeViewers.js, and, for supported types, modifications to viewer specifications inhttps://github.com/kbase/NarrativeViewers.Javascript changes
In
kbaseNarrativeWorkspace.js, thebuildViewerCellkbwidget method is modified to determine the narrative viewer before inserting the data cell. In the current implementation of data cells, which have not been touched, the viewer is determined by the python code. However, in order to insert a serviceWidget cell rather than a data cell, we need to inspect the viewer spec first. This is because we need to determine whether the requested viewer should be served as a dynamic service widget, or let the existing mechanism handle it. And we need to do this before inserting the cell.The narrative viewer specs are cached, although the first time a data object is inserted into the Narrative in a given session, there will be some delay as it is fetched from the Narrative Method Store.
buildViewerCelluses the existingloadViewerInfofunction in thenarrativeViewersmodule. The relevant function,loadViewerInfo, was not used in the codebase. I suspect it was used at one point, and then the functionality moved into the backend python handler for the data cell.loadViewerInfowas updated to utilize the app panel's version tag, so that one can utilize adev-tagged viewer. This allows us to have existing viewers operate as normal through thereleasetag, but if a user (in CI at least, when we are testing) selects thedevtag, use a newer narrative viewer spec that has been updated to invoke a service widget. (This is of course contingent upon how theNarrativeViewers"app" is managed via the catalog.)The viewer spec's
widget.outputproperty contains a special value for service widgetsServiceWidget. It acts as a pseudo-widget, as it does not correspond to an actual AMD viewer module like the extant data widget mechanism does. It is used for conditionally branching to code which prepares and invokes a service widget, as opposed to a standard viewer.The service widget itself is agnostic with respect to how the service widget will be used. We don't want to put viewer logic into the service widget code itself. Thus,
buildViewerCellis reponsible for packaging up the relevant widget invocation information, based on the viewer spec and the target object, so that the service widget cell can serve as a viewer cell. This informaiton includes the module name for the dynamic service, the widget name within the dynamic service, and the parameters, specifically the objectref.Service Widget NarrativeViewer Changes
To enable a service widget to serve as a data object viewer for a given type, the viewer spec for the type must be modified in a specific manner.
spec.json'swidgets.outputmust be"ServiceWidget"spec.jsonmust provide the module and widget names:constant_valuewith atarget_propertyof"service_module_name"constant_valuewith atarget_propertyof"widget_name"For example,
{ "name" : "View Media", "ver" : "1.0.0", "authors" : [ ], "contact" : "https://www.kbase.us/support/", "visible" : true, "categories" : ["viewers"], "app_type" : "viewer", "widgets" : { "input" : null, "output" : "ServiceWidget" }, "parameters" : [ ], "behavior" : { "none" : { "output_mapping" : [ { "constant_value": "eapearsonWidgetTest10", "target_property": "service_module_name" }, { "constant_value": "media_viewer_py", "target_property": "widget_name" } ] } } }Note that no parameters are required. The data object's ref is automatically generated within
buildViewerCelland passed to the widget. Thus, if you are modifying an existing viewer spec to use a service widget, you may safely remove any parameters.The
display.yamlrequires no changes to support service widgets, other than any description changes that may reflect changed functionality in the viewer. Of course, if parameters were removed fromspec.json, they need to be removed fromdisplay.yamlas well.Service Widget as App Output Viewer
Service widgets may also serve as viewers for app output.
As for data viewers, enabling service widgets requires changes to the existing mechanism for handling app output.
This functionality resides within the app cell itself - specifically the
createOutputCellfunction innbextensions/appCell2/appCellWidget.js.Similar to the data viewer cell, the implementation of the output viewer hinges on interceding before the cell type currently handling app output is inserted into the Narrative.
And also similar to the data viewer cell, the app output specification requires a specific format to indicate that a service widget should be used, and provides the service widget's dynamic service module name and widget name.
Summary of file changes
nbextensions/appCell2/appCellWidget.jsmodified to support inserting a service widget if the app spec specifies itkbase-extension/static/kbase/js/util/icon.jsmodified to add an app output icon based on the app icon itselfJavascript Changes
In the app cell execution lifecycle, an output cell may be inserted after the app has successfully completed. Whether to insert an output widget, which one to insert, and what parameters to provide it are all specified in the ui configuration for the app itself. We'll discuss that in the section below.
It is in the app cell implementation that the output cell is inserted into the Narrative, just below the app cell itself. The code responsible for this is in the
createOutputCellfunction innbextensions/appCell2/appCellWidget.js.The code previously had simply inserted an output cell. To support inserting a widget cell instead required few changes, as all relevant information was already available. The widget name is inspected, and if it matches
"ServiceWidget"will generate aserviceWidgetcell specification rather than anoutputwidget.Icon Changes
Icon support in
jsutils/icons.jswas extended to provide support for an "app output" icon. The current app output icon is a left-pointing arrow. In order to better associate the output cell with the app cell that generated it, the icon for app output viewers implemented as service widgets is a compound icon with the original app icon on the left, and on the right a "right-pointing" arrow.This change is speculative. I feel we need a better way of associating an output cell with it's "parent" app cell, but I'm not sure this is it.
App Requirements
As described above, the Javascript implementation of app output cell injection is straightforward, requiring few effective lines of code to be changed or added. These changes, however, rely upon a specific structure in the app's ui configuration. These changes are very similar to that for data viewers.
In the
ui/narrative/methods/APP_NAME(whereAPP_NAMEis the name of the app you are implementing) directory ofhttps://github.com/kbase/NarrativeViewers, thespec.jsonfile has the following requirements:widgets.outputmust be"ServiceWidget"behavior.service-mapping.output_mappingmust provide:constant_valuewith atarget_propertyof"service_module_name"constant_valuewith atarget_propertyof"widget_name"target_property"title"target_property"subtitle"All other outputs, other than the module name, widget name, title, and subtitle, are passed directly to the service widget as parameters.
For example, from an example app in CI (https://github.com/eapearson/eapearsonNarrativeViewersTest):
{ "ver": "0.0.2", "authors": [ "eapearson" ], "contact": "[email protected]", "categories": ["active"], "widgets": { "input": null, "output": "ServiceWidget" }, "parameters": [ { "id": "genome_ref", "optional": false, "advanced": false, "allow_multiple": false, "default_values": [ "" ], "field_type": "text", "text_options": { "valid_ws_types": ["KBaseGenomes.Genome"] } }, { "id": "scientific_name", "optional": true, "advanced": false, "allow_multiple": false, "default_values": [ "" ], "field_type": "text" }, { "id": "output_genome_name", "optional": false, "advanced": false, "allow_multiple": false, "default_values": [""], "field_type": "text", "text_options": { "valid_ws_types" : [ "KBaseGenomes.Genome" ], "is_output_name":true } } ], "behavior": { "service-mapping": { "url": "", "name": "eapearsonDemoGenomeEditClassification", "method": "update_genome_classification_metadata", "input_mapping": [ { "narrative_system_variable": "workspace_id", "target_property": "workspace_id" }, { "input_parameter": "genome_ref", "target_property": "genome_ref" }, { "input_parameter": "scientific_name", "target_property": "scientific_name" } ], "output_mapping": [ { "service_method_output_path": [0, "output_genome_ref"], "target_property": "output_genome_ref" }, { "service_method_output_path": [0, "change_log"], "target_property": "change_log" }, { "service_method_output_path": [0, "title"], "target_property": "title" }, { "service_method_output_path": [0, "subtitle"], "target_property": "subtitle" }, { "constant_value": "eapearsonWidgetTest10", "target_property": "service_module_name" }, { "constant_value": "demo_genome_edit_classification_viewer", "target_property": "widget_name" } ] } }, "job_id_output_field": "docker" }Developer Tools
In early development, in the days before the data viewer and app output integrations had been added, a developer tool was created to enable direct insertion of service widgets into a Narrative.
Feature detection
The developer tool is enabled through the existing feature detection mechanism, with one enhancement - supplying a set of enabled features in the URL. This addition allows one to enable a feature on the fly, rather than having to modify
config/features.json(or use Jupyter userSettings - but I don't know if we support it).The code for supporting feature detection and the developer feature in particular, is present in
common/ui.js. However, the UI object requires a DOM node and where we need feature detection it doesn't make any sense to pass a DOM node to the UI object.So the necessary code was moved to
common/runtime.js, which seems a more appropriate home, as the Runtime object does not require a DOM node. A few other small changes were made to the runtime module, for the sake of simplifying the code and module.The existing code in
ui.jswas refactored to call the code inruntime.js. I looked for existing usages of the feature detection code, and it wasn't fully clear how it was used, although it is not used widely, and some of the code that uses it is not currently in use.Below are the existing features and their usage. Some use the features configuration directly and are unafected by the feature api changes. Such calls look like
Config.get('features').staticNarratives.Two utilize the feature api calls, but appear to be unused. Well,
isDeveloper()andifAdvanced()are utilized in a few locations in the view cell widget code, but the view cell widget is not used.ifAdvanced() used in viewCellWidget but code using it is not displayed
config('features.developer') also used in viewCellWidget.js, code using them is not displayed
In our case, the feature we are interested in is
"developer", which we can enable in the URLhttps://ci.kbase.us/narrative/71571?features=developerOr in
kbase-extension/static/kbase/config/feature-config.json:{ "developer": true }Ad-hoc service widget tool
In developer mode, a new "developer" menu item appears on the top horizontal menu. It drops down two menu items:
Selecting either of these will display a dialog box in which the service widget parameters are entered.
Implementing this involved the following file changes:
kbase-extension/kbase_templates/narrative_header.htmladds the new dropdown menu. It is hidden by default, will be shown if the narrative is loaded with the 'developer' flag setkbase-extension/static/kbase/js/kbaseNarrative.jsprovides support for enabling the menu, as well as handling the menu items, as it does for other header menu items.static/kbase/js/util/bootstrapDialog.jsreceived an update to support both the dialogsizeas well as theshow.bs.modalevent. These changes are required in order to show and handle the service widget developer tool pop-ups.Other developer affordances
In addition, because I needed to insert and re-run service widget cells repeatedly, and manipulate cells extensively, I added a few conveniences that are enabled in developer mode:
Jira Ticket / Issue
Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-X
DATAUP-69 Adds a PR template)Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)