Feature/api definitions #171

kirilpopov · 2017-05-31T10:31:56Z

This is an attempt to define an API to unify what we currently have in ContainerJS, the working group APIs
and things that we at Tick42 think are useful based on our experience.

This is an initial version that aims to start a discussion - some parts of it are not fully defined (e.g. window events) and some
upcoming parts are just mentioned in comments (e.g. interop).

This also contains a document describing how to map the [the working group APIs in confluence (https://symphonyoss.atlassian.net/wiki/display/WGDWAPI/Proposed+Standard+API+Specifications) to the current API.

The starting point when reading the code should be ssf.ts file that describes the ssf root object as exposed to clients applications.

This reverts commit ca5ae28. # Conflicts: # packages/api-specification/api/windows.ts

BenLambertNcl

Looks like a great start!

I have left a few comments, mostly high level, but i'm not sure how much leeway we have in changing some of these APIs. I will do another code review once we have finalized on the design :-).

BenLambertNcl · 2017-05-31T12:57:33Z

packages/api-specification/api/windows.ts

+    capture(options?: CaptureOptions): Promise<Base64ImageData>;
+
+    /** Get the bounds of the window */
+    getBounds(): Bounds;


I think all window methods should be asynchronous, as OpenFin methods are all async.

They are, although I do wonder whether we should wrap them to be synchronous? All the equivalent Electron methods are synchronous.

I wonder whether this is an OpenFin implementation detail that has 'leaked' into the public API?

Usually these are sync operations , not sure why openfin did it that way.
Also I think usage is better

//move my window with 10px window.setBounds({left: window.getBounds().left + 20}); //vs window.getBounds().then((bounds)=>{ window.setBounds({left: bounds.left + 20}); });

BenLambertNcl · 2017-05-31T13:00:03Z

packages/api-specification/api/windows.ts

+export interface Window extends EventEmitter {
+
+    /** Get or set the max width of the window. Might be undefined if no restrictions applied**/
+    maxWidth?: number;


I think these will need to be getters/setters as they can change after the window is open. OpenFin's methods are also async, see below.

Yes these can be implemented as properties with getter and setters. (I prefer this over get* set* methods),

window.maxWidth = 10; console.log(window.maxWidth);

What do you think?

Not sure it would work, unless we have them as read only and use them instead of get methods. If we had an open window and did window.maxWidth = 10;, then we need a way of calling the underlying method to set the value. I think the only way to do this would be to watch for variable changes? Unless you can think of another way? :-)

I was thinking of using properties, something like (in typescript)

class Window{ private _maxWidth: number; get maxWidth():boolean { return this._maxWidth; } set maxWidth(width:number) { // do some container specific magic this._maxWidth= width; } }

Ah yeah I forgot we were using TypeScript 😄. Sounds like a good idea to me.

BenLambertNcl · 2017-05-31T13:08:06Z

packages/api-specification/api/common.ts

@@ -0,0 +1,20 @@
+// contains common types
+export interface Bounds {
+    top: number;


In Electron, top and left are x and y. If we are trying to stick to the Electron API as much as possible then maybe this should be changed?

BenLambertNcl · 2017-05-31T13:20:49Z

packages/api-specification/api/windows.ts

+     * Send a message to the window.
+     * @param {string|object} message - The message to send to the window. Can be any serializable object.
+     */
+    sendMessage(message: string | object): Promise<void>;


The HTML5 API is postMessage() so we should maybe use that?

Ok will update

BenLambertNcl · 2017-05-31T13:40:03Z

packages/api-specification/api/windows.ts

+     getById(windowId: string): Window;
+
+     /** Opens a new window */
+    open(options: WindowOptions): Promise<Window>;


I prefer using the constructor pattern that is used in some of the newer browser APIs (like notifications). Might be better to keep it like this though if window.open is used in the symphony application.

BenLambertNcl · 2017-05-31T13:42:46Z

packages/api-specification/api/windows.ts

+export interface WindowsAPI {
+
+    /** Returns the current window */
+    current: Window;


If we have getById(), then we might want to have these as getCurrent() and getFocusedWindow() etc. this would match both Electron + OpenFinas well.

Yes these should be get* methods . Will update

BenLambertNcl · 2017-05-31T13:48:27Z

packages/api-specification/api/application.ts

@@ -0,0 +1,33 @@
+export interface ApplicationAPI {


We might need to clarify the meaning of Application for OpenFin, as currently OpenFin windows that are not child windows are created using fin.desktop.Application(). Would this API be used for each OpenFin application, or ContainerJS application? If we want this per ContainerJS application, then we might need to rethink the setBadgeCount() or the window creation implementation as each ContainerJS application could have multiple top level windows, which could have different badge counts.

I guess a wider question is, should the API support multiple applications?

With OpenFin you can create a new Application instance through the JavaScript API.

Personally I think this is a bit of an edge case. I can't see when you'd want to do this in practice? I'd be happy with ContainerJS treating the Application as a singleton.

I didn't know that.
We should decide if we want to have access to other applications and be able to start some of them (however I'm not sure how other apps will be discovered in Electron case)

I think we can keep it as is for the time being, and have this as a single ContainerJS application. We can always change it in the future if we hit any problems

in general need to think about which aspects are mandatory and optional

ColinEberhardt · 2017-06-01T08:53:45Z

packages/api-specification/api/ssf.ts

+    /**
+     * Notifications API - publish notifications to the user
+     */
+    notifications: NotificationsAPI;


I guess the interesting thing here is that both within the browser and Electron container users will see two notification APIs:

new Notification(...) new ssf.Notification(...)

This is probably unavoidable, but something that users need to be warned about.

ColinEberhardt · 2017-06-01T08:54:52Z

packages/api-specification/api/system.ts

+     * If additional activity occurs within the period given by the 'throttle', then the callback will be
+     * invoked at time: X + throttle (where X is the time of last activity report send).
+     */
+    onUserActivity(callback: ()=> void, throtle: number);


thanks will change

ColinEberhardt · 2017-06-01T08:58:27Z

packages/api-specification/api/windows.ts

+    capture(options?: CaptureOptions): Promise<Base64ImageData>;
+
+    /** Get the bounds of the window */
+    getBounds(): Bounds;


They are, although I do wonder whether we should wrap them to be synchronous? All the equivalent Electron methods are synchronous.

I wonder whether this is an OpenFin implementation detail that has 'leaked' into the public API?

ColinEberhardt · 2017-06-01T08:59:42Z

packages/api-specification/api/windows.ts

+     * Receive notifications when a new window is opened
+     * @returns Function Execute the function to unsubscribe
+     */
+    onWindowOpened(callback: (window: Window) => void): UnsubscribeFunction;


Is there any reason why Window is an event emitter, whereas the WindowsAPI has explicit methods for each event type?

I'd prefer to stick to the event emitter pattern!

Ok will change

BenLambertNcl · 2017-06-01T14:39:13Z

packages/api-specification/api/windows.ts

+    focusedWindow: Window;
+
+    /**
+     getById(windowId: string): Window;


Missing closing comment

thanks - will update

…itions # Conflicts: # .gitignore

lneir · 2017-06-09T23:31:13Z

packages/api-specification/api/notifications.ts

+
+declare class Notification extends EventTarget {
+    constructor(title: string, options?: NotificationOptions);
+


looks like missing requestPermission method.

lneir · 2017-06-09T23:32:45Z

packages/api-specification/api/notifications.ts

+
+    close(): void;
+
+    readonly title: string;


also need likely need a sticky option

is this a standard field?

lneir · 2017-06-09T23:33:40Z

packages/api-specification/api/notifications.ts

+    close(): void;
+
+    readonly title: string;
+    readonly dir: NotificationDirection;


need to think how we might handle non-std fields. e.g., blink and blink color.

lneir · 2017-06-10T00:12:51Z

packages/api-specification/api/windows.ts

+ * It implements EventEmitter which means users can subscribe for events using on, once , addEventListener etc.
+ * The list of events is in WindowsEvents structure;
+ */
+export interface Window extends EventEmitter {


one issue i have here is that is kind of re-inventing the wheel. javascript already has a Window object and window.open nicely returns one these. so for javascript already using window.open this might require significant refactoring and might not be able to achieve same functionality. for example, window.opener. here the equivalent looks like window. getParentWindow() but it still don't think will give same functionality. what about postMesage and 'closed'

for example, one use case we have is we want child window to able to share large object (redux state) with parent. with window.opener this is possible. not sure here.

is it possible to augment existing window object (or inherit from it) and add missing/optional things we need here???

It's not about reinventing but for defining a common contract for container window. Note that this is not just about the current window, it's a way to address any window in your application from the current one (even if they don't have a parent/child relationship).

Any container I've worked with (Electron, OpenFin, GlueDesktop (which is Tick42 CEF based container)) has similar contract for windows, because the standard browser windowing API is not good enough for desktop applications.

Augmenting the existing window is actually a proposal for implementation of this contract.

for example, one use case we have is we want child window to able to share large object (redux state) with parent. with window.opener this is possible. not sure here.

One problem you'll encounter here is that the process model differs between various containers / browser.

Within a browser, a window opened via window.open is in the same process, therefore window.opener from the child process uses the same JavaScript context, and as a result, sharing objects, such as redux state becomes possible.

This is also true within OpenFin.

However, in Electron each renderer window has its own process, so you cannot just use window.opener to access the parent context and state.

This is why projects such as https://github.com/samiskin/redux-electron-store exist.

Within ContainerJS we are looking to create a consistent process model across all containers, as well as a consistent API.

using window.open (with sandbox on) in electron like this works just fine, the child can use the window.opener. we have this working currently in electron. when window.open url has the same domain, then it runs in the same process and the child can access the parent.

but yes if in diff process then window.opener won't work without some extra method to proxy between process across ipc.

Yes, we recently discovered this behaviour, and @BenLambertNcl is exploring it further via #195

lneir · 2017-06-10T00:14:37Z

packages/api-specification/api/system.ts

+     * Capture
+     * @param options
+     */
+    capture: (options?: CaptureOptions) => Promise<Base64ImageData>;


how is this capture working? would like to see something similar to ScreenSnippet here https://symphonyoss.atlassian.net/wiki/display/WGDWAPI/ScreenSnippet+API

lneir · 2017-06-10T00:16:03Z

packages/api-specification/api/system.ts

+
+export interface SystemAPI {
+    /**
+     * Info about all displays available


other interface needed is for screen sharing... https://symphonyoss.atlassian.net/wiki/display/WGDWAPI/getMediaSources+API which is essentially equal to https://electron.atom.io/docs/api/desktop-capturer/#desktopcapturer

lneir · 2017-06-10T00:19:33Z

packages/api-specification/api/messages.ts

+ * TBD - Request/response and streaming
+ */
+export interface MessagesAPI{
+    send(windowId: string, topic :string, message: string|object): void;


what is windowId here?

lneir · 2017-06-10T00:21:28Z

packages/api-specification/api/windows.ts

+     * Send a message to the window.
+     * @param {string|object} message - The message to send to the window. Can be any serializable object.
+     */
+    sendMessage(message: string | object): Promise<void>;


how to receive a message? ??? win.addEventListener('message', callback);

lneir · 2017-06-10T00:22:04Z

packages/api-specification/api/ssf.ts

+    /**
+     * Messaging API - communicate with other windows
+     */
+    messages: MessagesAPI;


how is this diff than window.sendMessage (postMesage) below?

Kiril Popov and others added 23 commits May 26, 2017 16:46

WIP

601c6f1

comment update

9d2df4c

typo

c5ec81d

comment

618fff7

WIP

05996d3

messages update

692cffe

updated openfin link

dc3e0ec

WIP

3499feb

WIP

ec42429

WIP

336f251

Updates

4ef36d0

Updates

79ce8b6

WIP

017c641

WIP

5e6e4cd

WIP

ca5ae28

updated

24c1dc7

readme

397ab4f

updated readme.md

a1c780d

updated readme

98491cd

Revert "WIP"

96bd363

This reverts commit ca5ae28. # Conflicts: # packages/api-specification/api/windows.ts

max* to be optional

8c17293

most thigns in WindowOptions should be optional

2da0091

Merge branch 'master' into feature/api-definitions

9203533

BenLambertNcl reviewed May 31, 2017

View reviewed changes

ColinEberhardt reviewed Jun 1, 2017

View reviewed changes

BenLambertNcl reviewed Jun 1, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/api-defin…

f603530

…itions # Conflicts: # .gitignore

lneir reviewed Jun 9, 2017

View reviewed changes

lneir reviewed Jun 10, 2017

View reviewed changes

ColinEberhardt mentioned this pull request Jun 13, 2017

Add symphony compatibility project #199

Merged

BenLambertNcl mentioned this pull request Jul 12, 2017

Add method to get window by id #243

Merged


		declare class Notification extends EventTarget {
		constructor(title: string, options?: NotificationOptions);

Feature/api definitions #171

Are you sure you want to change the base?

Feature/api definitions #171

Conversation

kirilpopov commented May 31, 2017 • edited

BenLambertNcl left a 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

BenLambertNcl Jun 5, 2017 • edited

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

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

BenLambertNcl Jun 5, 2017 • edited

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

kirilpopov Jun 5, 2017 • edited

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

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

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

kirilpopov commented May 31, 2017 •

edited

BenLambertNcl Jun 5, 2017 •

edited

BenLambertNcl Jun 5, 2017 •

edited

kirilpopov Jun 5, 2017 •

edited