Skip to content

Commit

Permalink
Adding initial changes.
Browse files Browse the repository at this point in the history
  • Loading branch information
Chris Montrois committed Aug 22, 2013
1 parent d1860d2 commit e2dfa28
Show file tree
Hide file tree
Showing 3 changed files with 153 additions and 9 deletions.
3 changes: 2 additions & 1 deletion Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ module.exports = function(grunt) {
'sdk/src/events.js',
'sdk/src/rpc.js',
'sdk/src/ui.js',
'sdk/src/container.js'
'sdk/src/container.js',
'sdk/src/amd.js'
]
},
less: {
Expand Down
151 changes: 151 additions & 0 deletions sdk/src/amd.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
// Check for the presence of AMD
if (typeof define !== 'undefined' && define.amd) {
// Define F2 as a module
define('F2', function() {
return F2;
});

// Define the F2App plugin
define('F2App', function() {

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

I'd prefer

define('F2/plugins/App', ['F2', function(F2) {

so that

  1. all future plugins go under F2/plugins, and
  2. passing in F2 rather than relying on the initial F2 in closure scope is friendlier to F2 plugins per #121

This comment has been minimized.

Copy link
@markhealey

markhealey Aug 30, 2013

Member

I could also argue this plugin shouldn't be called "app" since it's really an app integration plugin for Container Developers. Perhaps "AppLoad"?

http://docs.openf2.org/container-development.html#app-integration

This comment has been minimized.

Copy link
@montlebalm

montlebalm Aug 30, 2013

Member

"App" seemed appropriate because loading is in the very nature of RequireJS/AMD. The purpose of plugins is to load different types of dependencies. Consider the names of other widely used plugins: "css", "text", "json".

My interpretation is that the "require" statement itself is a stand-in for the word "load".

Our choice of plugin name should always be the most clear while keeping with the libraries conventions. If adding "load" will make it more easily understandable, we should definitely do so.

// Track batchable apps
var batchQueue = [];

// Get one token to share across all events
var token = F2.AppHandlers.getToken();

// Store appHandlers for every loaded appId
var userHandlers = {};

// Add an appHandler that will execute a user specified callback
function addAppHandler(eventName) {

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

Is there a coding convention that private functions that stay in the closure scope and aren't visible from outside should be prefixed with "_"?

This comment has been minimized.

Copy link
@Raylehnhoff

Raylehnhoff Aug 30, 2013

Contributor

Crockford suggests using them to indicate privacy, though they don't guarantee it. This SO article talks to this a bit.

This comment has been minimized.

Copy link
@montlebalm

montlebalm Aug 30, 2013

Member

I typically use the underscore convention when I'm exposing said functions outside the current context. I usually don't bother if no one will have programmatic access to them.

We can certainly change the naming scheme if it conflicts with F2's stated policy.

This comment has been minimized.

Copy link
@markhealey

markhealey Aug 30, 2013

Member

Leading underscores are preferred when denoting private funcs. See the style guide.

This comment has been minimized.

Copy link
@montlebalm

montlebalm Aug 30, 2013

Member

Do those standards apply to functions and methods inside closures as well? If so, we might have a few thousand offenders in the core codebase.

This comment has been minimized.

Copy link
@markhealey

markhealey via email Aug 30, 2013

Member
F2.AppHandlers.on(token, F2.Constants.AppHandlers[eventName], function(appConfig) {
var appId = appConfig.appId;

if (userHandlers[appId] && userHandlers[appId][eventName].length) {
// Pull off the first handler for this app
var handler = userHandlers[appId][eventName].shift();

if (handler instanceof Function) {
// Call handler with original arguments
var args = Array.prototype.slice.apply(arguments);
handler.apply(window, args);
}
}
});
}

// Add global event handlers
for (var eventName in F2.Constants.AppHandlers) {
if (F2.Constants.AppHandlers.hasOwnProperty(eventName)) {
addAppHandler(eventName);
}
}

// Look at all the specified appConfigs and see if one of them matches the appId
function getAppConfigById(configs, appId) {

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

Most libraries have some sort of findBy(array_of_objects, field, value) that can be used for this functionality:

function getAppConfigbyId(configs, appId){
  return findBy(configs, "appId", appId);
}

This comment has been minimized.

Copy link
@montlebalm

montlebalm Aug 30, 2013

Member

I'm not aware of such a function in F2, but I would gladly use it. We could bring in a 3rd party lib, but we'd have to decide if it's worth the KB.

var appConfig;

if (configs && appId) {
for (var i = 0; i < configs.length; i++) {
if (configs[i].appId == appId) {
appConfig = configs[i];
break;
}
}
}

return appConfig;
}

// Get an obj that will let the user add appHandlers
function getHandlerHooks(handlerMap) {
var handlerHooks = {};

// Break out this function or jsHint will yell at us
function addHook(eventName) {
if (!handlerMap[eventName]) {
// We'll store the handlers in an array, so we can handle multiple instances of the same app
handlerMap[eventName] = [];
}

handlerHooks[eventName] = function(fn) {
handlerMap[eventName].push(fn);
};
}

// Add a hook for each event
for (var eventName in F2.Constants.AppHandlers) {
if (F2.Constants.AppHandlers.hasOwnProperty(eventName)) {
addHook(eventName);
}
}

return handlerHooks;
}

// Actually pass the appConfig(s) into 'registerApps'
function loadApps(appConfig) {
// Batch if possible
if (appConfig.enableBatchRequests) {
if (batchQueue.length) {
F2.registerApps(batchQueue);
batchQueue = [];
}
}
else {
// Load the app individually
F2.registerApps(appConfig);
}
}

return {
load: function(uniqueAppId, req, onload, config) {

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

It's unclear to me why "onload" is a parameter. I can see it being used in line 137, but is it ever different than windows.onload or the framework's onload method?

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

config is only used if F2 is not yet initialized, right? Change following code to only check for config if !F2.isInit(), so config becomes just an optional param.

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

The reqparam doesn't seem to be used?

This comment has been minimized.

Copy link
@montlebalm

montlebalm Aug 30, 2013

Member

The "onload" param that appears in the "load" function is provided by the RequireJS plugin API. Executing the function tells RequireJS that your plugin is done and proceeds to run the callback to the "require" statement. For example:

require(["F2App!com_markit_appid"], function(app) {
    // "onload" triggers this function
});

This comment has been minimized.

Copy link
@montlebalm

montlebalm Aug 30, 2013

Member

Since F2App is a RequireJS plugin, we are bound to the provided API (http://requirejs.org/docs/plugins.html). As such, we are not in control of the parameters to the load function.

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 31, 2013

Ah, I completely missed that. OK, I'll have to take a look at that API, though at first glance, it looks identical to the AMD specification for loader plugins. However, if the intent of this is to be a RequireJS plugin, then it should be reflected in the name passed to define, say 'F2/plugins/require-loader' or something similar.

if (!config.f2) {
throw ('No require.config.f2 configuration found');
}

if (!config.f2.appConfigs) {
throw ('No appConfigs found.');
}

// Start f2 if we haven't already
if (!F2.isInit()) {
F2.init(config.f2.initConfig);
}

// Strip off the randomizer guid and get the config
var appId = uniqueAppId.substring(0, uniqueAppId.indexOf('?'));

if (!appId) {
throw ('appId is empty');
}

var appConfig = getAppConfigById(config.f2.appConfigs, appId);

if (!appConfig) {
throw (appId, 'is not a recognized appId.');
}

// Add this as a possible batch request
if (appConfig.enableBatchRequests) {
batchQueue.push(appConfig);
}

// Get our handler hooks and tell the require statement to execute the callback
userHandlers[appId] = userHandlers[appId] || {};
var handlerHooks = getHandlerHooks(userHandlers[appId]);
onload(handlerHooks);

// Load apps in a timeout so we can batch
setTimeout(function() {
loadApps(appConfig);
}, 0);
},
normalize: function(appId, normalize) {

This comment has been minimized.

Copy link
@ilinkuo

ilinkuo Aug 30, 2013

Hm. I wonder if this might solve the race condition in loading multiple instances of apps with the same appID? Will have to check.

// Randomize the module name
// This allows us to load the same app multiple times and get different instances
return appId + '?' + F2.guid();
}
};
});
}
8 changes: 0 additions & 8 deletions sdk/src/template/footer.js.tmpl
Original file line number Diff line number Diff line change
@@ -1,12 +1,4 @@

exports.F2 = F2;

if (typeof define !== 'undefined' && define.amd) {

define(function() {
return F2;
});

}

})(typeof exports !== 'undefined' ? exports : window);

10 comments on commit e2dfa28

@ilinkuo
Copy link

@ilinkuo ilinkuo commented on e2dfa28 Sep 1, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long-winded comment but I am of the opinion that this plugin takes us further away from our goals rather than nearer ...

Where we (TDA) ultimately want to be with F2 and AMD is to have F2 replace its custom loader with AMD's. The advantages of this are:

  1. Duplicate resource loading is eliminated for multiple instances. The current F2 loader, if it were to load 5 instances of the Streaming News Module, would load all its js and css resources five times.
  2. Dependency Traversal. If Streaming News and StockTwits were AMD modules which used a library which in turn depended on d3, then requireJS would figure out that d3 only needed to be loaded once.
  3. AMD encourages modular javascript at the price of cutting the javascript up into many small files. To counter this, AMD loaders include a concatenation utility such as r.js to traverse the dependency tree and concatenate all dependencies into a single file.
    Example chart app not drawing in IE8 and IE7 #1 and Watchlist app needs error handling for Yahoo API failures #2 are achieved through the use of a cache whose key is the normalized resource name. The current F2-requireJS plugin undercuts this through the normalize() method implementation. normalize("streaming_news") returns something like "streaming_news?GUID1234567". So, instead of reducing the cache key space, this normalize() expands it and reduces the cache hit ratio to effectively zero. As a result, this ensures that resources will be redundantly loaded.

So, suppose the implementation of normalize is changed, would that fix this problem? Well, the behavior seems intentional so I'm not sure it really can be fixed. Even if it could somehow fixed, I would argue that the approach of creating a requireJS plugin is not the right approach because it seems to me to require a large detour. Correcting the normalize (if it can be corrected) merely delegates the loading to F2's loading engine -- it does not give the RequireJS loader visibility into the dependencies so that it can traverse the dependency tree and weed out duplicates. It also does not give the r.js optimizer the capablity to figure out what dependencies to concatenate together.

From our point of view, we don't want syntactic sugar that makes an F2App an uncacheable AMD resource which is loaded using F2 loading engine. We want F2 to switch out its loading engine to AMD, something like http://jsfiddle.net/RTXg3/7/

As the example shows, this can be done while preserving the form of the manifest and most of its semantics. By examining the network tab in the console, note that duplicate resources are only loaded once by the jsfiddle.

@montlebalm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilinkuo I think I can address a few issues you brought up.

First, preventing duplicate asset loading is simple. There is already an effort underway to add this as an option to F2. In the instance of normalize, it's possible for the app is loaded N times but the assets only once.

Second, changing the normalize function to return a cached result would be as easy as deleting the GUID. If we wanted that to be the default behavior (as it is for RequireJS), we could add an overload syntax to allow for multiple app instantiations (e.g., F2App!com_markit_appId:new).

Third, my reference to RequireJS is only showing my ignorance when it comes to AMD. I am tangentially aware of projects like Dojo and Almond, but I don't know how similar they all are. My gut is that we could easily build this plugin to work for most implementations of AMD loaders.

Lastly, writing a plugin does not preclude the possibility of an AMD based version of F2. To do it right, you'd want to rethink F2 from the ground up and build it to be a first class citizen with AMD. That might be the approach that best serves F2 and its users, but it is a big effort and cannot be undertaken lightly.

In the end, if this plugin makes users lives easier, then I think it should be explored until we can find a more permanent solution.

Thanks for your comments.

@ilinkuo
Copy link

@ilinkuo ilinkuo commented on e2dfa28 Sep 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, preventing duplicate asset loading is simple. There is already an effort underway to add this as an option to F2. In the instance of normalize, it's possible for the app is loaded N times but the assets only once.

The point is to not expend the effort to add this to F2 because it's a dead end path. If the assets are all loaded by RequireJS, then RequireJS handles this. If two different F2 apps need d3.js, then with this functionality, the F2 loader can avoid loading d3.js twice. However, if an F2 App needs d3.js and the AMD'ed F2 Container also needs d3.js, how does the F2 loader engine communicate to the RequireJS that d3.js is already loaded? The only way for this to happen is for RequireJS/AMD to load both.

Second, changing the normalize function to return a cached result would be as easy as deleting the GUID. If we wanted that to be the default behavior (as it is for RequireJS), we could add an overload syntax to allow for multiple app instantiations (e.g., F2App!com_markit_appId:new).

So AFAIK, removing the GUID from normalize() doesn't actually work, and this is what I alluded to when I said I didn't know when I said I didn't know how to fix it. For one, the F2App plugin's cached value is actually the handler hooks -- see line #137 onload(handlerHooks) -- which don't have anything to do with the App. Secondly, you have an assumption that with the following code:

require(["F2App!Hello_World"], function(){ /* ... */});
// do other stuff
require(["F2App!Hello_World"], function(){ /* ... */});

that the load() function on line #102 will actually get called twice, and thus have the side effect of invoking loadApps() on line #87 twice. I may be wrong, but as far as I know, that won't happen. After the first invocation of require(), AMD will catch the return value from the onload(handlerHooks) call and skip the second invocation. Thus, the F2 App Hello_World will only get loaded once when we want two instances on the page.

Third, my reference to RequireJS is only showing my ignorance when it comes to AMD. I am tangentially aware of projects like Dojo and Almond, but I don't know how similar they all are. My gut is that we could easily build this plugin to work for most implementations of AMD loaders.

Both RequireJS and Dojo's AMD loader are written by James Burke, who is also the main driver behind the AMD specification, so I agree that very few changes, if any, would be needed.

Lastly, writing a plugin does not preclude the possibility of an AMD based version of F2. To do it right, you'd want to rethink F2 from the ground up and build it to be a first class citizen with AMD. That might be the approach that best serves F2 and its users, but it is a big effort and cannot be undertaken lightly.

While I agree it should not be taken lightly, I would say it's much easier than you would think. Please take a look at the fiddle (http://jsfiddle.net/RTXg3/7/) from my previous response. It already handles the main task of loading (but not the peripheral ones such as secure loading, batch loading, etc.) in a way that's similar to the existing F2._loadApps(). The changes are transparent to current users because the manifest is not altered.

In the end, if this plugin makes users lives easier, then I think it should be explored until we can find a more permanent solution.

I'm fine with making the users' lives easier. What I'm objecting to is dragging AMD into this when this plugin has nothing really to do with AMD and brings none of AMD's advantages. A much simpler solution would be to just take the body of the load() method above as the body of a new F2.easyLoad() method. That way, the user still has the convenience and AMD/RequireJS isn't involved at all.

@ilinkuo
Copy link

@ilinkuo ilinkuo commented on e2dfa28 Sep 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to expand on the example in my previous comment to make the point why I think an F2App plugin for RequireJS is the wrong thing to do.

require(["F2App!Hello_World"], function(app){ /* ... */});
// do other stuff
require(["F2App!Hello_World"], function(app){ /* ... */});

The question to ask, is how should this snippet behave?

Let's look at the behavior of some of the plugins from https://github.com/jrburke/requirejs/wiki/Plugins.

require(["text!some/module.html", "json!some/module.json", "mdown!some/module.md", "css!styles/main"], function(text, json, html) {
    // the text variable will be the text of the some/module.html file
    // the json variable will be the parsed json of the some/module.json file
    // the html variable will be the contents of the markdown file transformed into html
    // there is no fourth parameter for the css variable since that is directly loaded onto the page
});
require(["text!some/module.html", "json!some/module.json", "mdown!some/module.md", "css!styles/main"], function(text, json, html) {
    /* ... */
});

For the text, json, and mdown plugin, the result is retrieved, processed and then cached by the first require. The second require call merely retrieves the cached result from the first require when it becomes available. The css plugin doesn't cache anything because it loads directly onto the page, but the second css require doesn't reload the result onto the page again. The AMD plugins all work this way because they all load resources in an idempotent manner -- multiple invocations of a resource through RequireJS have the same effect as a single invocation.

Going back to the F2App snippet, again we ask "How should this behave?"

require(["F2App!Hello_World"], function(app){ /* ... */});
// do other stuff
require(["F2App!Hello_World"], function(app){ /* ... */});

The first require loads the Hello_World onto the page. If the second require loads a second instance of the Hello_World onto the page, then it's acting differently from every other RequireJS plugin out there because it's not idempotent, and you're subverting the spirit of AMD. If the second require conforms to idempotency and doesn't do anything, then it's not really loading an F2 App.

This is the problem posed by an F2 RequireJS plugin that I don't see any out for.

@montlebalm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a fundamental difference as to how require() is meant to be used and how containers want to load F2 apps. Loading an app in the spirit of require means being limited to a single instance, or else forcing the container to manually instantiate the AppClass. The purpose of this plugin was to provide a simpler way to load apps for developers already versed in AMD. I agree that the current plugin behaves differently than other existing plugins, but I've never seen any official suggestion that this needs to be the case.

In my mind, loading apps in the "spirit of AMD" looks something like this:

require(["F2App!com_domain_appId"], function(appClass) {
  var app = new appClass();
  app.init(/* pass in root, ui, instanceId, etc */);
});

I think the additional dependencies you'd need just call init() make this approach a little clunky.

To do a proper port of F2, every AppClass needs to be wrapped in a define() block with dependencies that are also wrapped in define(). These dependencies will need to be referenced by path, which likely can't be hard-coded because of your build system (i.e., for cache busting). Dependencies need to be named explicitly by version, because apps may depend on different 3rd party libraries. If path aliases are defined, containers and app developers need to agree on a naming convention or else every app needs to return every lib it needs, thereby inflating the size of every app request.

Porting F2 to AMD is a larger undertaking than changing the codebase, which itself is a significant task. It's possible that it's easier than I think, but that doesn't mean it's easy.

If you think it's possible to load F2 apps via AMD, please provide an example of how that would look to a container. Using require() to load scripts is fine, but you need to call it on the AppClass to get the actual benefit of dependency sharing.

@brianbaker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this plugin is just a wrapper for loading apps in F2, much like the goog plugin is a wrapper for Google APIs. F2App isn't changing the internals of F2 just like goog isn't changing the internals of Google Loader; F2App defers to F2.registerApps() and goog defers to google.load(). What those internal methods do (whether its in the spirit of AMD or not) is up to those separate libraries. Like @montlebalm said, its just (possibly) a simpler way to load apps.

If we want to talk about fundamental change in F2 and switching out the internal script loader to use proper AMD, then that discussion could probably go in a separate GitHub issue or even on Google Groups. I think switching out the internal script loader is out of the scope of this plugin. To really do it properly, its likely going to result in changes to how existing apps are written like @montlebalm mentioned. Its likely going to be difficult or impossible to use proper AMD with F2 v1.0; it'll probably have to be in an F2 v2.0.

@markhealey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plenty of discussion on this specific commit, so I'll keep it here rather than moving it over to #125.

The goal in writing the code in this branch amd-plugin is to do just that—introduce a requirejs plugin for defining apps "the require way". Actually, I prefer @montlebalm's "spirit of AMD" better. The goal was not to replace F2's internal script loader with an AMD loader. What this means of course (@ilinkuo pointed this out) is that F2 apps are still loaded the "F2 way" and, as of #120, that way is fairly unintelligently appending script tags to the <body>. This proposed requirejs plugin simply becomes a Container Developer's shortcut for integrating F2 apps into a solution where requirejs is already present. It effectively removes the necessary step Container Developers have to take in writing F2 container configuration code.

As part of version 1.2.2—tracked in #120—the internal script loader replaced jQuery.ajax with document.createElement('script') per a couple of discussions on #11. This was a necessary step to take while we continue to evaluate a larger scale replacement of the internal loader and dependency management in general. We're going to go ahead and release 1.2.2.

In terms of next steps:

  1. If we can address the normalize issues, I don't have a problem adding this requirejs plugin to F2's core. Use it if you want to, leave it if you don't. It doesn't address @ilinkuo's specific needs but it does satisfy other requests from unnamed parties. @montlebalm, please take some feedback from @ilinkuo and update your pull request (e.g., normalize, renaming it require-loader or similar, etc).
  2. Separately, I'd like to see us start a new thread (GitHub Issue) outlining requirements for replacing F2's internal loader with an AMD model. There are numerous items to discuss and @montlebalm touches on a couple of them in his last comment. If there's a means of adding a new loader based on requirejs adjacent to F2's current loader (switched on by a config param for F2.init) without changing AppManifest formats thus forcing a version 2.0.0, I'm all for exploring that in the short term.

@ilinkuo
Copy link

@ilinkuo ilinkuo commented on e2dfa28 Sep 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brianbaker goog gets away with loader delegation because there are no shared dependencies between the require caller (your code) and the require loadee (google apis). With no shared dependencies, it doesn't matter if different loader engines are used. In F2's case, the require caller (Container code?) and the require callee (App code) can have shared dependencies, so using loader delegation results in redundant dependency loading.

@ilinkuo
Copy link

@ilinkuo ilinkuo commented on e2dfa28 Sep 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@montlebalm Your suggestion of

require(["F2App!com_domain_appId"], function(appClass) {
  var app = new appClass();
  app.init(/* pass in root, ui, instanceId, etc */);
});

is a plugin I would support. The differences between this and the existing proposal:

  • Its action is idempotent like the other plugins.
  • The cached value is the app class rather than the handlers
  • It loads the module's resources rather than the module instance
  • With the existing proposal, I cannot see a path where it could be iterated to a good solution (in my opinion). With the revised API above, I can see a path to iterate (see my reply to @markhealey in the next comment).

To do a proper port of F2, every AppClass needs to be wrapped in a define() block with dependencies that are also wrapped in define(). These dependencies will need to be referenced by path, which likely can't be hard-coded because of your build system (i.e., for cache busting). Dependencies need to be named explicitly by version, because apps may depend on different 3rd party libraries. If path aliases are defined, containers and app developers need to agree on a naming convention or else every app needs to return every lib it needs, thereby inflating the size of every app request.

These problems exist in the current F2, they're just ignored. There's nothing in to prevent F2 Apps from loading incompatible versions of d3.js, for instance. Every app currently loads every lib it needs, regardless of any others. Solving these problems are not a prerequisite to an F2 AMD port. It's the other way around, in my opinion -- moving to AMD is a prerequisite to being able to deal with these problems. This is why I've been pushing hard for F2 to switch out its engine to AMD from when I first got involved (Oct 2012).

I agree that a proper port of F2 requires wrapping everything in define(), but you can get pretty far with unwrapped scripts. See http://ilinkuo.wordpress.com/2013/02/07/dojo-amd-incorporating-third-party-scripts/

@ilinkuo
Copy link

@ilinkuo ilinkuo commented on e2dfa28 Sep 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markhealey

This proposed requirejs plugin simply becomes a Container Developer's shortcut for integrating F2 apps into a solution where requirejs is already present. It effectively removes the necessary step Container Developers have to take in writing F2 container configuration code.

My point is, why dress the shortcut up in AMD lipstick when the plugin isn't really using AMD and it's acting in a way different from all other AMD plugins? Why not just add a convenience method with the same functionality to F2 itself without involving AMD at all? That way, both AMD'ed code and non-AMDed code could invoke it.

My preferred course of action would be:

  1. Take the existing plugin's functionality and put it in a new method F2.easyLoad().
  2. Decide to write an F2/RequiresJS plugin in the style of the revised plugin API which @montlebalm proposed
 require(["F2App!com_domain_appId"], function(appClass) {
  var app = new appClass();
  app.init(/* pass in root, ui, instanceId, etc */);
 });
  1. Implement this plugin with the simplest loading scenario and check in the code to 1.3-wip under an F2X namespace.
  2. Quickly iterate, adding batch loading, secure loading, resource naming conventions, etc. until until it is feature complete/equivalent with the existing F2 loader.
  3. In 1.4 or whenever the plugin is feature complete, replace the underlying engine of the legacy API with the plugin's engine.
  4. In 2.0 documentation, mark the legacy API as deprecated, slated for removal in 3.0. This step is optional, as F2 can decide to maintain both APIs for backwards compatibility.

Doing an in-place rewrite the existing F2 engine is difficult and carries a lot of risk. The plan above creates an alternate AMD path and allows for gradual migration for both F2 itself and its users. This plan also brings AMD into F2 much much sooner than the official roadmap.

Please sign in to comment.