Revert "Merge pull request #18 from PAIR-code/dev"

This reverts commit ba7ad82, reversing
changes made to d0ba035.

documentation/frontend_development.md

@@ -106,10 +106,9 @@ outlined below:
 @customElement('demo-module')                                                   // (0)
 export class DemoTextModule extends LitModule {
   static override title = 'Demo Module';                                        // (1)
-  static override template =
-      (model: string, selectionServiceIndex: number, shouldReact: number) =>    // (2)
-          html`<demo-module model=${model} .shouldReact=${shouldReact}
-                selectionServiceIndex=${selectionServiceIndex}></demo-module>`;
+  static override template = (model = '') => {                                  // (2)
+    return html`<demo-module model=${model}></demo-module>`;
+  };
   static override duplicateForModelComparison = true;                           // (3)
 
   static override get styles() {
@@ -137,7 +136,7 @@ export class DemoTextModule extends LitModule {
     this.pigLatin = results;
   }
 
-  override renderImpl() {                                                       // (10)
+  override render() {                                                           // (10)
     const color = this.colorService.getDatapointColor(
         this.selectionService.primarySelectedInputData);
     return html`
@@ -209,14 +208,14 @@ of the data. Since we're using mobx observables to store and compute our state,
 we do this all in a reactive way.
 
 First, since the `LitModule` base class derives from `MobxLitElement`, any
-observable data that we use in the `renderImpl` method automatically triggers a
+observable data that we use in the `render` method automatically triggers a
 re-render when updated. This is excellent for simple use cases, but what about
 when we want to trigger more complex behavior, such as the asynchronous request
 outlined above?
 
-The pattern that we leverage across the app is as follows: The `renderImpl`
-method (10) accesses a private observable `pigLatin` property (6) that, when
-updated, will re-render the template and show the results of the translation
+The pattern that we leverage across the app is as follows: The `render` method
+(10) accesses a private observable `pigLatin` property (6) that, when updated,
+will re-render the template and show the results of the translation
 automatically. In order to update the `pigLatin` observable, we need to set up a
 bit of machinery. In the lit-element lifecycle method `firstUpdated`, we use a
 helper method `reactImmediately` (7) to set up an explicit reaction to the user
@@ -225,10 +224,7 @@ selecting data. Whatever is returned by the first function (in this case
 second function immediately **and** whenever it changes, allowing us to do
 something whenever the selection changes. Note, another helper method `react` is
 used in the same way as `reactImmediately`, in instances where you don't want to
-immediately invoke the reaction. Also note that modules should override
-`renderImpl` and not the base `render` method as our `LitModule` base class
-overrides `render` with custom logic which calls our `renderImpl` method for
-modules to perform their rendering in.
+immediately invoke the reaction.
 
 We pass the selection to the `getTranslation` method to fetch the data from our
 API service. However rather than awaiting our API request directly, we pass the
@@ -272,48 +268,11 @@ reconciliation of what needs to be updated per render.
     this.drawCanvas(canvas);
   }
 
-  override renderImpl() {
+  render() {
     return html`<canvas></canvas>`;
   }
 ```
 
-### Stateful Child Elements
-
-Some modules may contain stateful child elements, where the element has some
-internal state that can have an effect on the module that contains it. Examples of this include any modules that contain the
-[elements/faceting_control.ts](../lit_nlp/client/elements/faceting_control.ts) element.
-
-With these types of child elements, it's important for the containing module
-to construct them programmatically and store them in a class member variable,
-as opposed to only constructing them in the module's html template
-string returned by the `renderImpl` method. Otherwise they will be destroyed
-and recreated when a module is hidden off-screen and then brought back
-on-screen, leading them to lose whatever state they previously held.
-Below is a snippet of example code to handle these types of elements.
-
-```typescript
-// An example of a LITModule using a stateful child element.
-@customElement('example-module')
-export class ExampleModule extends LitModule {
-  private readonly facetingControl = document.createElement('faceting-control');
-
-  constructor() {
-    super();
-
-    const facetsChange = (event: CustomEvent<FacetsChange>) => {
-      // Do something with the information from the event.
-    };
-    // Set the necessary properties on the faceting-control element.
-    this.facetingControl.contextName = ExampleModule.title;
-    this.facetingControl.addEventListener(
-        'facets-change', facetsChange as EventListener)
-  }
-
-  override renderImpl() {
-    // Render the faceting-control element.
-    return html`${this.facetingControl}`;
-  }
-```
 ## Style Guide
 
 *   Please disable clang-format on `lit-html` templates and format these

lit_nlp/client/core/faceting_control.ts

@@ -54,16 +54,7 @@ export interface FacetsChange {
 
 type FeatureChangeHandler = (event?: Event) => void;
 
-/**
- * Controls for defining faceting behavior.
- *
- * This faceting control should be created using the call
- * `document.createElement('faceting-control')"` or `new FacetingControl()` and
- * stored as a data member of any module that wishes to make use of it, as
- * opposed to being invoked through element templating in a `renderImpl` method
- * call. This is done so that options set in this control aren't reset when a
- * module's render is skipped due to the module being off-screen.
- */
+/** Controls for defining faceting behavior. */
 @customElement('faceting-control')
 export class FacetingControl extends ReactiveElement {
   private readonly featureConfigs = new Map<string, FacetingConfig>();

lit_nlp/client/core/lit_module.ts

@@ -76,8 +76,8 @@ export abstract class LitModule extends ReactiveElement {
 
   // Template function. Should return HTML to create this element in the DOM.
   static template:
-      (model: string, selectionServiceIndex: number,
-       shouldReact: number) => TemplateResult = () => html``;
+      (model: string,
+       selectionServiceIndex: number) => TemplateResult = () => html``;
 
   @property({type: String}) model = '';
   @observable @property({type: Number}) selectionServiceIndex = 0;
@@ -109,36 +109,6 @@ export abstract class LitModule extends ReactiveElement {
     });
   }
 
-  /**
-   * Base module render function - not to be overridden by clients.
-   *
-   * This render function will call the renderImpl method if the module is set
-   * to react, otherwise it will not render anything.
-   *
-   * Any client overridding this method will not get the standard behavior of
-   * pausing rendering when a module is set to not react. This may cause issues
-   * as the module will still have reactions paused in this case. Therefore,
-   * clients should avoid overidding this method and instead they should
-   * implement renderImpl.
-   */
-  override render() {
-    // If the module is not reactive, then do not render anything.
-    if (this.shouldReact === 0) {
-      return;
-    }
-    return this.renderImpl();
-  }
-
-  /**
-   * Render function for each LIT module to override.
-   *
-   * Only called if the module is reactive, meaning it is visibly on-screen.
-   * Clients should override this method as opposed to the base render() method.
-   */
-  protected renderImpl(): unknown {
-    return html``;
-  }
-
   /**
    * A helper method for wrapping async API calls in machinery that a)
    * automatically sets the loading state of the parent widget container and

lit_nlp/client/core/modules.ts

@@ -395,9 +395,8 @@ export class LitModules extends ReactiveElement {
       const classes = classMap({selected, 'components-group-holder': true});
       return html`
         <div class=${classes}>
-          ${
-          this.renderWidgetGroups(
-              configs, tabName, layoutWidths, `${idPrefix}-${i}`, selected)}
+          ${this.renderWidgetGroups(configs, tabName, layoutWidths,
+                                    `${idPrefix}-${i}`)}
         </div>`;
     });
   }
@@ -425,9 +424,8 @@ export class LitModules extends ReactiveElement {
     });
   }
 
-  renderWidgetGroups(
-      configs: RenderConfig[][], section: string, layoutWidths: LayoutWidths,
-      idPrefix: string, visible: boolean) {
+  renderWidgetGroups(configs: RenderConfig[][], section: string,
+                     layoutWidths: LayoutWidths, idPrefix: string) {
     // Recalculate the widget group widths when isMinimized state changes.
     const onMin = () => {
       this.calculatePanelWidths(section, configs, layoutWidths);
@@ -492,8 +490,7 @@ export class LitModules extends ReactiveElement {
       // clang-format off
       return html`
         <lit-widget-group id=${id} .configGroup=${configGroup} .width=${width}
-                          @widget-group-minimized-changed=${onMin}
-                          ?visible=${visible}>
+                          @widget-group-minimized-changed=${onMin}>
         </lit-widget-group>
         ${isLastGroup ? html`` : html`
             <div class="expander" style=${expanderStyles}>

lit_nlp/client/core/widget_group.ts

@@ -15,7 +15,6 @@
  * limitations under the License.
  */
 
-// tslint:disable:no-new-decorators
 import '../elements/checkbox';
 import '../elements/spinner';
 import '@material/mwc-icon-button-toggle';
@@ -26,7 +25,6 @@ import {html} from 'lit';
 import {customElement, property} from 'lit/decorators';
 import {classMap} from 'lit/directives/class-map';
 import {styleMap} from 'lit/directives/style-map';
-import {observable} from 'mobx';
 
 import {ReactiveElement} from '../lib/elements';
 import {styles as sharedStyles} from '../lib/shared_styles.css';
@@ -61,7 +59,6 @@ export class WidgetGroup extends ReactiveElement {
   @property({type: Boolean, reflect: true}) minimized = false;
   @property({type: Boolean, reflect: true}) maximized = false;
   @property({type: Boolean, reflect: true}) dragging = false;
-  @observable @property({type: Boolean, reflect: true}) visible = false;
   @property({type: Number}) width = 0;
   private widgetScrollTop = 0;
   private widgetScrollLeft = 0;
@@ -223,18 +220,18 @@ export class WidgetGroup extends ReactiveElement {
       config: RenderConfig, styles: {[key: string]: string},
       showSubtitle: boolean) {
     const moduleType = config.moduleType;
-    const modelName = config.modelName || 'unused';
-    const selectionServiceIndex = config.selectionServiceIndex || 0;
-    const shouldReact = this.visible && !this.minimized;
+    const modelName = config.modelName;
+    const selectionServiceIndex = config.selectionServiceIndex;
+
     let subtitle = modelName ?? '';
     /**
      * If defined, modules show "Selected" for 0 and "Pinned" for 1,
      * If undefined, modules do not show selectionService related info in their
      * titles (when compare examples mode is disabled)."
      */
-    if (moduleType.duplicateForExampleComparison) {
+    if (typeof selectionServiceIndex !== 'undefined' && moduleType.duplicateForExampleComparison) {
       subtitle = subtitle.concat(`${subtitle ? ' - ' : ''} ${
-          selectionServiceIndex ? 'Pinned' : 'Selected'}`);
+        selectionServiceIndex ? 'Pinned' : 'Selected'}`);
     }
     // Track scolling changes to the widget and request a rerender.
     const widgetScrollCallback = (event: CustomEvent<WidgetScroll>) => {
@@ -255,8 +252,7 @@ export class WidgetGroup extends ReactiveElement {
         widgetScrollTop=${this.widgetScrollTop}
         style=${styleMap(styles)}
       >
-        ${moduleType.template(modelName, selectionServiceIndex,
-          shouldReact ? 1 : 0)}
+        ${moduleType.template(modelName, selectionServiceIndex)}
       </lit-widget>
     `;
     // clang-format on

lit_nlp/client/lib/elements.ts

@@ -21,14 +21,9 @@
 
 // tslint:disable:no-new-decorators
 import {MobxLitElement} from '@adobe/lit-mobx';
-import {property} from 'lit/decorators';
-import {IReactionDisposer, IReactionOptions, IReactionPublic, observable, reaction} from 'mobx';
+import {IReactionDisposer, IReactionOptions, IReactionPublic, reaction} from 'mobx';
 
 type ReactionInputFn<T> = (r: IReactionPublic) => T;
-// tslint:disable:no-any
-type PendingReactionMap =
-    Map<(arg: any, r: IReactionPublic) => void, {arg: any, r: IReactionPublic}>;
-// tslint:enable:no-any
 
 /**
  * Extension of MobxLitElement, with automatic reaction disposal.
@@ -42,51 +37,15 @@ export abstract class ReactiveElement extends MobxLitElement {
   // TODO(b/204677206): remove this once we clean up property declarations.
   __allowInstanceProperties = true;  // tslint:disable-line
 
-  /**
-   * Indicates if the element should perform reactions. Set to 0 if the
-   * element is not visible to the user and any other number if it is visible.
-   * Not using a boolean here as web components cannot have boolean attributes
-   * that default to true if not specified due to how boolean attributes are
-   * passed to elements, given that we want this attribute to default to true
-   * for backwards compabitibility with elements that don't have this value set
-   * through an attribute.
-   */
-  @observable @property({type: Number}) shouldReact = 1;
-
   private readonly reactionDisposers: IReactionDisposer[] = [];
 
-  // Map of pending reactions that are stored when an element is in a state
-  // where it shouldn't react which are performed when the element is set to
-  // react again.
-  private readonly pendingReactions: PendingReactionMap = new Map();
-
-  constructor() {
-    super();
-    reaction(() => this.shouldReact, shouldReact => {
-      if (shouldReact === 0) {
-        return;
-      }
-      this.performPendingReactions();
-    });
-  }
-
   override disconnectedCallback() {
     super.disconnectedCallback();
     this.reactionDisposers.forEach(disposer => {
       disposer();
     });
   }
 
-  /**
-   * Performs all pending reactions and clears them from the map.
-   */
-  private performPendingReactions() {
-    this.pendingReactions.forEach((value, effect) => {
-      effect.apply(window, [value.arg, value.r]);
-    });
-    this.pendingReactions.clear();
-  }
-
   /**
    * A simple wrapper method around mobx `reaction`. Note that this reaction
    * is not called immediately (for that, use this.reactImmediately).
@@ -96,18 +55,7 @@ export abstract class ReactiveElement extends MobxLitElement {
   protected react<T>(
       fn: ReactionInputFn<T>, effect: (arg: T, r: IReactionPublic) => void,
       opts: IReactionOptions = {}) {
-    // Wrapper function to pass to the mobx reaction call. If the element is
-    // set to react, then the effect happens immediately. If not, the arguments
-    // for the effect are stored in the map, keyed by the effect, so that
-    // the latest reaction of each type is stored for later use.
-    const reactWrapper = (arg: T, r: IReactionPublic) => {
-      if (this.shouldReact !== 0) {
-        effect.apply(window, [arg, r]);
-      } else {
-        this.pendingReactions.set(effect, {arg, r});
-      }
-    };
-    const disposer = reaction(fn, reactWrapper, opts);
+    const disposer = reaction(fn, effect, opts);
     this.reactionDisposers.push(disposer);
   }
 

lit_nlp/client/lib/types.ts

@@ -283,8 +283,7 @@ export type ServiceUser = object;
 export interface LitModuleClass {
   title: string;
   template:
-      (modelName: string, selectionServiceIndex: number,
-       shouldReact: number) => TemplateResult;
+      (modelName?: string, selectionServiceIndex?: number) => TemplateResult;
   shouldDisplayModule: (modelSpecs: ModelInfoMap, datasetSpec: Spec) => boolean;
   duplicateForExampleComparison: boolean;
   duplicateForModelComparison: boolean;