Skip to content
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

Extendable API #58

Open
4shub opened this issue May 18, 2019 · 6 comments
Open

Extendable API #58

4shub opened this issue May 18, 2019 · 6 comments
Labels
enhancement New feature or request

Comments

@4shub
Copy link

4shub commented May 18, 2019

I was wondering if there were plans to create an extensible API so users can additional types of applications. I would be very interested in contributing to something like this.

I am working on a fork of this project to allow users to demo a web browser experience using your base code. I wouldn't want that browser window to be added to this repo as it would bloat the app unnecessarily for most people, but I think lots would value if they could add it as an option if we added some sort of API to add extra types of applications.

Let me know what you think and I can submit a PR possibly :)

@4shub 4shub changed the title Extendable API Creating an extendable API May 18, 2019
@4shub 4shub changed the title Creating an extendable API Extendable API May 18, 2019
@rafaelcamargo rafaelcamargo added the enhancement New feature or request label May 19, 2019
@rafaelcamargo
Copy link
Member

This is a nice suggestion. I myself already have been thinking about this exactly by the need to demonstrate a couple of action on a browser.

However, I confess that I have not thought any way of implementing this. The plugin strategy seems very interesting to me. If you like to propose some implementation strategy, please go ahead. It will be a pleasure to me to discuss that 👍

@4shub
Copy link
Author

4shub commented May 19, 2019

Sure, I was thinking of making a very simple extension API that acts unobtrusively with the existing API.

Something like the following:

// sample.js
import GDemo, { extender } from '@glorious/demo';

extender({ name: 'browser', content: BrowserApplication });

The content in this case would be a similarly structured Application file as you have for Terminal and Editor (we probably would just need to create a type file or something for devs)

This extender would ultimately just add an application type to some list we create in the existing desktop.js

// desktop.js
const ApplicationTypes = {
  editor: EditorApplication,
  terminal: TerminalApplication,
};

export const extender = (newApplicationType) => {
  ApplicationTypes[newApplicationType.name] = newApplicationType.content;
};

// existing code
function buildApplication(desktop, applicationType, options){
  const Application = ApplicationTypes[applicationType] || TerminalApplication;
  const application = new Application(desktop.element, options);
  desktop.openApplications.push(application);
  desktop.element.appendChild(application.element);
  return application;
}

This way I feel is a little better than just invoking in let's say new GDemo('selector', options) because we would assume if a user wanted to extend their gDemo with new application types, they would probably want to just set it once and be done with it.

What do you think?

@rafaelcamargo
Copy link
Member

Interesting approach. Two things that I liked:

  1. Clear and simple to implement.
  2. In fact, seems to solve peculiarities that are beyond the scope of the lib.

Some concerns:

  1. openApp starts to be used differently depending on the app to be open.
  2. The specific app just created does not inherit features that are related to an Application. e.g.: windowTitle, minHeight, etc. This may break expectations related to the acceptable options.

What about the following?

import GDemo from '@glorious/demo';
import browserApp from 'gdemo-browser-app'; // client implementation or a package published on NPM

GDemo.addApp(browserApp);

const gDemo = new GDemo('#selector');

gdemo
  .openApp('browser', { minHeight: '400px', windowTitle: 'Github' })
  .loadPage(...) // example of app specific method
  .end();

That way, the implementation of browserApp must extend the base Application:

// gdemo-browser-app.js
import { Application } from '@glorious/demo';

export class BrowserApplication extends Application {
  constructor(options) {
    super('browser', options);
  }
  loadPage() {
    // app specific method 
  }
}

The changes on Desktop.js seems to be very close to the changes you have suggested. Perhaps, we need to create a service to retrieve applications by application name from an application enum.

What do you think?

@4shub
Copy link
Author

4shub commented May 20, 2019

That looks good to me, and I agree that we should have some sort of service to retrieve these applications because I realized that we would also have to extend index.js to allow for users to add new custom-app-specific methods.

We would have to do a small rewrite where we have to force the user to invoke openApp before running any method that isn't openApp, and make the app attribute in the steps just rely on the last called 'openApp' to determine what app we need to run the application specific method from.

export default class gDemo {
  constructor(selector){
    this.container = document.querySelector(selector);
    this.steps = [];
    this.currentApp = null;
  }
  openApp(app, options = {}){
    this.currentApp = app;
    this.steps.push({
      app,
      options,
      onCompleteDelay: options.onCompleteDelay
    });
    return this;
  }
 // invoke functions from plugins
  do(actionName, params, options = {}){
    this.steps.push({
      app: this.currentApp,
      action: actionName,
      params,
      onCompleteDelay: options.onCompleteDelay
    });
    return this;
  }
}

And also write some method in Application that handles invocations of functions that do not exist

throw new Error(`${fnName} is not a method of the ${appName} application`);

@rafaelcamargo
Copy link
Member

Right, I see your point. Well, below is how exactly works the flow of opening/closing applications:

  1. First of all, every animation step is registered in an Array containing the details of each step. At this phase, no animation is still playing.
  2. When user invokes .end(), the array of steps is passed to the Player which is responsible to play every step following its details.
  3. The Player instantiate a Desktop. The Desktop is the only component that manages applications. It builds, maximizes (opens) or minimizes applications when Player asks it.
  4. When Desktop brings the application to the Player, Player executes the application method informed in the current step.

The problem is that every application method is interfaced through GDemo class methods. If GDemo class has no custom application methods, the steps for this custom application couldn't be registered.

That said, I have some concerns on your approach:

I realized that we would also have to extend index.js

  1. If user extends GDemo class to add custom application methods to it, we take the risk of having method name collision - one plugin could potentially break another 😢

So, what if the custom application (plugin) returned its own interface to register steps and added them to the steps on the instance of GDemo?

I am figuring out here how to make this in the Custom Application (plugin). As soon as I have one first solution, I put it here 👍

@rafaelcamargo
Copy link
Member

rafaelcamargo commented May 23, 2019

Take this as a simple brainstorm. I confess that I still don't like the way some Custom App is being implemented (seems too complex).

import GDemo from '@glorious/demo';
import { BrowserApp } from 'gdemo-browser-app'; // client implementation or a package published on NPM

GDemo.addCustomApp(BrowserApp);

const gDemo = new GDemo('#selector');

gdemo
  .openApp('browser', { minHeight: '400px', windowTitle: 'Github' })
  .loadPage(...) // example of app specific method
  .end();
// index.js
import customApplicationsService from './services/custom-applications/custom-applications';

export default class {
  static addCustomApp = function(App) {
    customApplicationsService.add(App);
  }
  constructor(selector){
    this.container = document.querySelector(selector);
    this.customApps = [];
    this.steps = [];
  }
  openApp(app, options = {}){
    if(isBuiltInApp(app)) {
      this.steps.push({
        app,
        options,
        onCompleteDelay: options.onCompleteDelay
      });
      return this;
    } else {
      return getCustomApp(app, options, this);
    }
  }
  ...
}

function isBuiltInApp(app) {
  return ['editor', 'terminal'].includes(app);
}

function getCustomApp(app, options, gDemo) {
  // Look up on gDemo.customApps. If found it, return it. Otherwise:
  const Application = customApplicationsService.find(app);
  const customApp = new Application(gDemo.steps, options);
  buildProxyForGDemoOpenAppInCustomApp(customApp, gDemo);
  buildProxyForGDemoEndInCustomApp(customApp, gDemo);
  gDemo.customApps.push(customApp);
  return customApp;
}

function buildProxyForGDemoOpenAppInCustomApp(customApp, gDemo) {
  customApp.openApp = function(app, options) {
     return gDemo.openApp(app, options);
  };
}

function buildProxyForGDemoEndInCustomApp(customApp, gDemo) {
  customApp.end = function() {
     return gDemo.end();
  };
}
// gdemo-browser-app.js
import { Application } from '@glorious/demo';

const app = 'browser';

export class BrowserApplication extends Application {
  static app = app;
  constructor(steps, options) {
    super(app, options);
    this.steps = steps;
  }
  loadPage(mainParams, options) {
    this.steps.push({
      app,
      params: mainParams,
      action: (params) => {
        // some implementation for loading a page
      },
      onCompleteDelay: options.onCompleteDelay
    });
    return this;
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants