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

Expand on F2.registerApps method for added app placement support #38

Closed
markhealey opened this issue Feb 28, 2013 · 21 comments
Closed

Expand on F2.registerApps method for added app placement support #38

markhealey opened this issue Feb 28, 2013 · 21 comments
Milestone

Comments

@markhealey
Copy link
Member

The goal here is to expand on F2.registerApps() to provide Container Developers more control over placement of F2 apps. This includes initial placement on-page-load as well as on more rich client apps where F2 apps are loaded on-demand after the initial page-load.

Two immediate additions are:

  • Adding a simpler mechanism for Container Developers to provide a DOM node via the AppConfig (currently handled by the appRoot argument).
  • Provide hooks for beforeAppRender(), appRender() and afterAppRender() through F2.registerApps(). Those handlers are currently only available in F2.init().

We'll expand with more comments and commits on this Issue.

@ghost ghost assigned brianbaker Feb 28, 2013
@ilinkuo
Copy link

ilinkuo commented Feb 28, 2013

This looks like it will solve my problem.

By AppConfig, I presume is meant the AppConfig argument to registerApps() method, not the private F2._apps kept internally by F2 itself ? It would not be good to allow direct modification of the _apps by the container code.

Also, the hooks for beforeAppRender(), etc. would be temporary overrides? The next call to registerApps() would use the default ones defined via F2.init()?

@markhealey
Copy link
Member Author

Thanks I-Lin—you're absolutely right. Read only makes sense... although we did kick around the idea of allowing Container code to add properties to the AppConfig—extending it, not overwriting it—when the AppConfig gets sent to F2.registerApps(). I can think of a couple of use cases but none that demand immediate inclusion of a feature like this, so maybe we'll circle back to it.

@Ali-Khatami
Copy link
Contributor

I would love to see something more along the lines of subscription/broadcast rather than a singular callback in Init() and eliminate beforeAppRender(), appRender(), afterAppRender() entirely (making those methods private to F2).

I was thinking something like:

/\* Way to handle all apps (Your container's default) _/ // don't pass AppID means apply to all F2.Container.AppRender.Before(function(app){ /_ call method for all apps_/ }); F2.Container.AppRender.Before(function(app){ /_ do this after because was bound second */ }); /\* Way to handle a specific app id, basically an override for specific App _/ F2.Container.AppRender.Before("SpecificAppID", function(app){ /_ do something special for this app only */ });

I would love to hear more thoughts on this...

We could even open this up to allow apps to have a say in how they are rendered (although you could just do something inside your js you return from in the manifest)

@ilinkuo
Copy link

ilinkuo commented Mar 2, 2013

@markhealey , regarding

Adding a simpler mechanism for Container Developers to provide a DOM node via the AppConfig

I would actually prefer the DOM node to be provided separately, not as part of AppConfig. This is to allow a more jQuery-flavored API. For example,

registerApp(appRoot, appConfig, handlers)

and allow the appRoot to be either a DOM node or a css string, just like jQuery. In the case of a css string, F2 is responsible for creating the actual domNode and adding it to the master appConfig. The handlers argument are a means to provide a temporary override for the xxxAppRender() methods for this call only. While this doesn't preclude reading the appRoot from appConfig if no appRoot argument is provided, I think the added "flexibility" detracts from the simplicity of the API with very little gain.

For the registerApps() method, while you could allow the appRoot to be read from each appConfig, I wouldn't. The registerApps() should be thought of as just a convenience for calling each registerApp() method individually and should function identically.

@brianbaker
Copy link
Member

@Ali-Khatami - If we went the subscription/broadcast route, what should happen if the container subscribed to both F2.Container.AppRender.Before(function() { ... }) and also F2.Container.AppRender.Before("AppID", function() { ... }) would both functions be called or just the app-specific one? Should the app-specific one be called first and then the global one or vice versa?

@Ali-Khatami
Copy link
Contributor

@brianbaker - It's a tough call... Below are the pros and cons of the 2 use cases I feel are most logical...

Case 1:
All Apps method runs then App specific method runs

Pros:

  • Allows "All Apps" to be more of a base render function which sets up generic structure/params
  • Allows App specific to not have to duplicate effort.
  • Better code reuse and fairly easy to understand
  • More useful for groups of apps or categories of apps where you may want all apps from a category to be wrapped a certain way and be placed somewhere generic, then the app specific can dictate order or extra functionality.

Cons:

  • Sometimes it may be extra processing for specific apps because it runs through base functions
  • May lead to confusion if multiple "All Apps" before methods are bound and run

Case 2:
App Specific method runs only if bound

Pros:

  • More useful for traditional web pages
  • Less overhead if pre-processing for all apps isn't necessary
  • Less confusing because a developer will know that only 1 method will be fired

Cons:

  • Less code reuse, more than likely base functionality will be farmed off to another function or just rewritten for each app

I personally feel that Case 1 is more useful and provides a better way to separate concerns and help reuse code. It is a design pattern a lot of object oriented programmers are used to.

Obviously Case 1 doesn't really work so hot if a container developer just wants to pass a native dom node...

F2.Container.AppRender.Before(nativeDOMNodeToPlaceContents)
F2.Container.AppRender.Before("AppID", nativeDOMNodeToPlaceContents)

I feel the dom node approach should be app specific. I can't imagine a scenario where a container developer can trust apps enough to just place ALL requested apps in a single DOM node without manipulating or wrapping them first.

So I think the 3 overloads should look like...

  1. F2.Container.AppRender.Before(function(){ ... }) Allows for pre-processing
  2. F2.Container.AppRender.Before("AppID", function(){ ... }) Allows for extra processing specific to an app
  3. F2.Container.AppRender.Before("AppID", nativeDOMNodeToPlaceContents) Allows for pre-processing in the base Before() but just tells F2 where to ultimately append a specific app.

The order should be Generic/All Apps method(s) are called, then App specific method(s) are called which can either be a function or a straight up native node telling F2 where append the app.

@ilinkuo
Copy link

ilinkuo commented Mar 6, 2013

None of the alternate proposals for overriding handlers meet our needs. AppRender is not the only method that needs to be overridden. beforeAppRender() and afterAppRender() need to be overridden too, based on the division of responsibility in the example container.js

  • beforeAppRender creates the wrapper -- app title and dropdown menu -- and attaches to the DOM
  • afterAppRender appends the app's html to the DOM

Our use case is that we want to allow the reader to drag and drop F2 Apps into different regions on the page. Based on the division of responsibility in the example container.js, both beforeAppRender() and afterAppRender() must be overridden in order to have the DOM information. Furthermore, they will be overridden differently for each invocation of registerApp and the DOM node will be different, so none of the following are sufficient

F2.Container.AppRender.Before(function() { ... })
F2.Container.AppRender.Before("AppID", function() { ... })

Furthermore, the DOM node might not be the only variation of the handlers needed between invocations, as there might be other styling differences. So far, the only proposal that solves our problem is the one I laid out

registerApp(appRoot, appConfig, handlers)

where the handlers are overriden at the time of invocation.

However, I see in the discussions a desire to interact with the base xxxAppRender methods and not just override them. That desire can be accommodated elegantly using the handlers object above. When a handlers object is received, set methods on the object _appRender, _beforeAppRender, _afterAppRender. If this is done, then the definitions of handler methods can refer to the existing methods and interact with it in any way you choose, not just in a before aspect.

{ 
    afterAppRender: function(app, html){
        html = "<div>" + html + "</div>";
        return this._afterAppRender(app, html);
    }
}

@brianbaker
Copy link
Member

None of the alternate proposals for overriding handlers meet our needs. AppRender is not the only method that needs to be overridden. beforeAppRender() and afterAppRender() need to be overridden too, based on the division of responsibility in the example container.js

I think the subscription/broadcast proposal was just looking at a single method for the sake of saving typing, but all methods could be overridden. For the sake of example (naming can be decided later - the names used below aren't good), you might have:

beforeAppRender() becomes F2.Container.AppRender.BeforeRender()
appRender becomes F2.Container.AppRender.Render()
afterAppRender() becomes F2.Container.AppRender.AfterAppRender()

Our use case is that we want to allow the reader to drag and drop F2 Apps into different regions on the page.

Not to get this thread side-tracked, but were you planning on calling removeApp() and then registerApps() any time a user drags an app on the page (thus needing to override the rendering quite a bit)? Or are you just going to move the apps around in the DOM without removing/re-registering?

So far, the only proposal that solves our problem is the one I laid out

registerApp(appRoot, appConfig, handlers)

where the handlers are overriden at the time of invocation.

To clarify a bit if I'm understanding the proposals discussed, I think both proposals might be able to do the same thing. The following might be an example of the two:

var appConfig = { appId: 'com_foo', ... };

// Proposal 1
F2.registerApps( $('#someSelector').get(0), appConfig, {
    beforeAppRender: function() { ... },
    appRender: function() { ... },
    afterAppRender: function() { ... }
});

// Proposal 2
F2.Container.AppRender.Before(appConfig.appId, $('#someSelector').get(0) );
F2.Container.AppRender.Before(appConfig.appId, function() { ... });
F2.Container.AppRender.During(appConfig.appId, function() { ... });
F2.Container.AppRender.After(appConfig.appId, function() { ... });
F2.registerApps(appConfig);

@ilinkuo
Copy link

ilinkuo commented Mar 7, 2013

To answer your question, Brian, for us, an F2 app might appear in one of several regions. In the Main region, the app can have all the width it wants. In the Dock, it's constrained to a width of ~160 pixels. When the user asks for an app, he/she might ask for it to be created in the lower left corner of the Main region or in the middle of the Dock, for example. This would involve a call to registerApp(). The primary problem here is that the proper app creation requires information that is only available at the time of the request.

If a user were to remove an app from the Dock and recreate it in the Main, then this would involve a removeApp() followed by a registerApp(). If the user were to simply drag-and-drop the app between regions, then ideally this would not involve any registerApp() or removeApp calls (though we might have to recreate the F2 app's container div.)

Now, going back to the Proposals,

// Proposal 1
F2.registerApps( $('#someSelector').get(0), appConfig, {
    beforeAppRender: function() { ... },
    appRender: function() { ... },
    afterAppRender: function() { ... }
});

would certainly work for us, but I think would require additional changes in the signature of the xxxAppRender() methods. I had been thinking more along the lines of

var targetNode = $('#someSelector').get(0);
F2.registerApps( appConfig, {
    beforeAppRender: function() { ... },
    appRender: function() { ... },
    afterAppRender: function() { ... }
});

where targetNode would be in the closures of the functions and would not require a signature change in the existing APIs. As for Proposal 2,

// Proposal 2
F2.Container.AppRender.Before(appConfig.appId, $('#someSelector').get(0) );
F2.Container.AppRender.Before(appConfig.appId, function() { ... });
F2.Container.AppRender.During(appConfig.appId, function() { ... });
F2.Container.AppRender.After(appConfig.appId, function() { ... });
F2.registerApps(appConfig);

I think that has to be modified to

// Proposal 2
try {
    F2.Container.AppRender.Before(appConfig.appId, $('#someSelector').get(0) ); //not sure how to clear this out
    F2.Container.AppRender.Before(appConfig.appId, function() { ... });
    F2.Container.AppRender.During(appConfig.appId, function() { ... });
    F2.Container.AppRender.After(appConfig.appId, function() { ... });
    F2.registerApps(appConfig);
} finally {
    F2.Container.AppRender.ResetBefore(appConfig.appId);
    F2.Container.AppRender.ResetDuring(appConfig.appId);
    F2.Container.AppRender.ResetAfter(appConfig.appId);
}

in order to clear out the overrides made before the registerApps() call. This should be done because the function arguments are relevant only for this particular instance of the app and not for any others.

So, while I agree that both proposals can provide the same functionality, I strongly disagree that the proposals are of equal merit. Indeed, the same functionality can even be done with the current release of F2 without any modifications to the framework itself, but it would be a hack in my opinion.

Allow me then to throw back a couple of questions:

  • What is your concrete use case justifying
F2.Container.AppRender.Before(appConfig.appId, function() { ... });
  • Why can't this code just be an if-then statement in the original beforeAppRender() provided to F2.init()? I mean, if you just want different kinds of apps to be handled differently, why not just code it in the original beforeAppRender() passed to F2.init()? In our case, it's because there's information in the closure of the function that is particular to this instance of the app that isn't available at the time of F2.init(). I suspect that would be the case in any use case you provide as well. In that case, we're really solving the same underlying problem.

@brianbaker
Copy link
Member

My primary concern with overloading registerApps() is that it doesn't provide a single entry point to handling rendering. It introduces a bit of fragmentation in that developers can either use F2.init() or F2.registerApps() to handle rendering (@Ali-Khatami's suggestion would also introduce fragmentation in v1 of F2, but v2 could remove the rendering callbacks from F2.init resulting in a single entry point). It also tightly couples the rendering of an app to the registration of an app to the container. If I have a module that allows a user to select what apps to be on the page, I don't want that module to also be in charge of how the app renders - there'd probably be a layout manager or something that would be in charge of that.

I like the thought of an event-based rendering system because it provides the single entry point for rendering. F2 already uses eventing for context passing and will likely use a similar system for secure messaging down the road. Additionally, web developers deal with dom events on a daily basis and this really would be no different. It meets the existing functionality of F2.init() and it also would allow for single-app customization. Also, it opens the door to other rendering scenarios ("Render all apps with an AppId of com_example_sidebar_* this way and render all apps with an AppId of com_example_main_* that way").

For the scenarios where you want to render something once and be done with it, instead of requiring a try/finally, why not provide something similar to F2.Events.once().

Perhaps the developer has one module that handles the layout of apps in the main column of the page and second module that handles the layout in a sidebar and a third module that provides the interface for a user to select apps. When the user selects an app, the third module would call F2.registerApps() for the app that was selected, but before doing so would add a property to the App's context which would tell modules one and two where the app was supposed to go (doing this would also tell the App what type of space constraints it might have).

@brianbaker
Copy link
Member

I put together an example of an event-based rendering system from my understanding of @Ali-Khatami's proposal without modifying F2.js: http://jsfiddle.net/rekabnairb/efyH4/

For the sake of time, I'm just using jquery events and I'm not handling all of the edge cases. If we chose to roll something like this up into the framework, we could certainly provide a better interface and handle all edge cases. I would also envision that the handlers in F2.init() would be removed in v2 in favor of just using a system like this (For v1, we could just internally remap any handlers passed into F2.init() into this system). Here is what it currently demonstrates:

  • No need for try/finally because .one() is used
  • Single-app granularity (through event namespaces)
  • Passing of data in appConfig.context
  • User can decide where the App ends up (or not)
  • Allows the rendering to be separated out from the actual F2.registerApps() call, allowing the rendering code to specialize on rendering and the app selection code to specialize on selection

There's one handler that only applies when a certain location is selected. I could see this type of logic being rolled up into the framework so that the framework handles the logic and conditionally calls the handler.

@ilinkuo
Copy link

ilinkuo commented Mar 11, 2013

It also tightly couples the rendering of an app to the registration of an app to the container.

I think that's a problem of the existing API (regardless of any overloading), where registerApps() does a lot more than just registration. I would recommend reserving the use of the verb "register" only for operations having to do with the upcoming F2 registry. Also, just to be absolutely clear, I'm not recommending overloading registerApps() but rather defining a new registerApp(). This balances out the asymmetry in the API between removaApp(), removeAllApps() vs registerApps(). I actually don't like the semantics of registerApps() and would avoid using it.

I like the thought of an event-based rendering system because it provides the single entry point for rendering.
Even after your jsFiddle example, I don't see how you come to this conclusion, but I won't contest it.

Also, it opens the door to other rendering scenarios ("Render all apps with an AppId of com_example_sidebar_* this way and render all apps with an AppId of com_example_main_* that way").

So ... this remains the major point of contention between us. I absolutely think the above is a trivial use case that can be adequately handled by a if-then-switch-case statement within the methods passed to F2.init(). The harder problem is the problem of providing just-in-time handlers to configure individual app instances. For us, branching the handling based on AppId is a dead end -- there is no advantage to be gained there, because every instance of every app has different parameters . For example, this StreamingNews instance may only be tuned to the latest 5 tech stock updates with a width of 120px. Another instance may catch the latest 10 banking news with a width of 180px, and in addition, those items for BoA highlighted with a red background. The location data is not the only parameter that is not known until the app is to be loaded. You may, as in your example, put all of these things into the appConfig even with F2 1.0, but I would consider that a hack because most of this just doesn't belong in AppConfig, and AppConfig will get bloated very quickly, IMHO. You may not see this as a problem since you recommend this as an approach. However, from my point of view, I am deliberately trying to avoid loading all the parameters willy nilly into AppConfig, but I'll go along if this is an F2 recommended practice.

I put together an example of an event-based rendering system from my understanding

It's an interesting example, and I can see that it does address the location-dependent loading, and while in the fiddle the containers are created ahead of time, it can be easily adjusted to use just-in-time dynamically created containers. However, one of its flaws is that it still assumes handling is based on AppID only, not app instance. This will cause problems if appRender() is slow and multiple instances of the same app are created at the same time, because the instances may intercept events meant for the other instances. This can't easily be solved by tacking an instance id onto the eventID since the instanceID is created by F2 and not available at the time of registerApps() call.

@brianbaker
Copy link
Member

Making use of AppConfig.context is definitely a supported practice and isn't a hack to do so. See http://docs.openf2.org/app-development.html#container-to-app-context-server Many, if not all, of your Just-In-Time instance concerns (width, height, and other runtime config) seem like perfect use cases for Context

The single entry point to rendering would be the event system. Developers wouldn't need to learn about handling rendering in F2.init or alternatively in F2.registerApp or F2.registerApps or even x when a new use case comes up.

And to your final point about multiple instances of the same app - I'm not sure of your exact use case and what logic needs to be performed for each instance, but I would wonder if that too would be solved with context?

@ilinkuo
Copy link

ilinkuo commented Mar 11, 2013

Since you're recommending passing of parameters using the AppConfig.Context, that's what we'll go ahead and do, despite our reservations with this approach. This will also solve our problems with multiple instances, though it does not, at first thought, manage to resolve the mis-interception of events problem raised earlier, but that's not something that needs to be resolved now.

So here are some clarifying questions/comments to this approach:

  • The docs say that

The appConfig object is sent to the server using the params querystring name as shown in the example below. This is the complete app manifest request sent by F2.registerApps() with the appConfig URL-encoded ...

My opinion is that some of this information is sensitive and should not appear in a plain URL. Additional guidelines need to be added to prevent the appearance of sensitive information in URL querystring in a uniform way across all containers.

  • The fact that appConfig is to be serialized as JSON and sent as a request parameter implies that any callback methods placed in appConfig must be small, and DOM nodes probably can't be placed in AppConfig. Also, other than parameters for authentication, the vast majority of parameters are only relevant for client-side, not server-side. So, these concerns point towards a necessity to censor the serialized appConfig object passed to manifest.js which isn't currently in the spec.
  • I question whether the AppConfig needs to be sent to manifest.js as a default. I actually prefer a static manifest.js response rather than a dynamic one for security reasons -- a static response allows me to compute a SHA1 on the manifest.js and all its contents so I can be sure that versions haven't changed without my knowledge.

@brianbaker
Copy link
Member

I think we'll handle these final questions via the newly created #46 if that's OK, as it seems like the framework needs a little more work in that area. I'll write up some comments over there and we'll keep this thread moving towards adding the expanded rendering functionality.

@brianbaker
Copy link
Member

I'd like to try and nail down the interfaces, namespaces, etc. and initial functionality for the subscription/broadcast (event) based rendering. Anyone have any suggestions? At one point, I thought that maybe we could use F2.Events but it isn't setup to handle return objects which we need for rendering. Here are the methods that I can think of that should probably be supported (namespace is a suggestion):

  • F2.Rendering.on(event/namespace, handler) - adds a listener with a function to execute when the event is fired
  • F2.Rendering.on(event/namespace, domNode) - shorthand for adding a listener that returns the domNode as the root for the app
  • F2.Rendering.on(event/namespace, condition, handler) - adds a listener with a condition which will first be fired to determine whether the handler should also be fired
  • F2.Rendering.one(...) - Same as .on() above, but the handler will only execute once
  • F2.Rendering.off() - removes all listeners
  • F2.Rendering.off(event/namespace) - removes a listener given an event/namespace
  • F2.Rendering.off(event/namespace, handler) - removes a listener given an event/namespace and a handler

@markhealey
Copy link
Member Author

@brianbaker is there anything from #4 that needs to be considered for this update?

@ilinkuo
Copy link

ilinkuo commented Mar 14, 2013

@brianbaker I know you're aware of this already, but just to point this out, you've got a mixture of event paradigms here. Most falls under simple pub/sub -- one publisher, multiple listeners, and the publisher doesn't really care what the subscribers do nor whether it is done synchronously or asynchronously. A variation of that is topic/channel-based pub/sub -- multiple publishers and listeners, but again the publishers don't care what the subscribers do. Most eventing libraries are setup to do pub/sub, which is why

but it isn't setup to handle return objects which we need for rendering.

While

F2.Rendering.on(event/namespace, domNode)

would also technically fall under "eventing", it's really very different from the other methods -- only one publisher and one subscriber, and the publisher must wait synchronously for the return value from the subscriber before proceeding. As such, I'm a little wary of an api that makes the different kinds of events look the same. I'd prefer a different terminology, maybe callback? http://en.wikipedia.org/wiki/Callback_(computer_science)

@brianbaker
Copy link
Member

@markhealey Definitely, and with that in mind my F2.Rendering naming convention isn't good. We'll need to support beforeAppRemove, appRemove, and afterAppRemove. I've also heard some interest in something like F2.reloadApp() as a way to reload/refresh an app (perhaps just reloading the html and re-init the app class) and that might mean beforeAppReload, appReload, afterAppReload.

@ilinkuo I think we could support multiple listeners/subscribers (my jsfiddle example had a few listeners on the beforeAppRender event), but they'd definitely be synchronous in execution and differ from F2.Events like you said.

Its also worth noting that this functionality really only applies to the Container so only a container developer would need to know the difference between F2.Events and F2.?. Maybe because of that, we go with a name that has "Container" in it like @Ali-Khatami originally suggested. F2.Container.Callbacks or F2.Container.Events or F2.Container.? (with the caveat that we need to boldly disclaim that this differs from F2.Events)?

@ilinkuo
Copy link

ilinkuo commented Mar 21, 2013

This can be closed, with the resolution that the recommendation is to pass app placement information into the appConfig context itself. Future clarification needed for a standard way of expressing this within the appConfig.

@brianbaker
Copy link
Member

Like #4, we need to get documentation written in the spec before we can release this.

@ghost ghost assigned Ali-Khatami Apr 23, 2013
brianbaker added a commit that referenced this issue Apr 25, 2013
brianbaker added a commit that referenced this issue May 6, 2013
Extra bug fixes and updates for #38
@brianbaker brianbaker mentioned this issue May 9, 2013
24 tasks
markhealey pushed a commit that referenced this issue Jun 4, 2013
Fixes for recommendations/requirements from #38 and issue #92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants