Pull Request for "AMD Plugin" branch #125

Open
wants to merge 10 commits into
from

Conversation

Projects
None yet
6 participants
@montlebalm
Member

montlebalm commented Aug 27, 2013

I've added an "F2App" plugin for RequireJS that will allow containers to load F2 apps more easily.

I have also made a handful of bug fixes including:

  • Removing all external dependencies from tests (files, servers, etc). Files were downloaded and remote servers were replaced by local Node instances.
  • Preventing F2 from lowercasing manifestUrls during batch requests. Casing is now preserved.
  • Syncing files and HTML structure between "/tests/index.html" and "/tests/index-amd.html".

The expected behavior from my changes has been encapsulated in a series of tests in "/tests/spec/amd-spec.js".

@markhealey

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Aug 28, 2013

Member

@montlebalm I'd like to merge this into a 1.2.3-wip branch (which hasn't been created yet) instead of master since this is likely to be launched with some other updates. Keep working in amd-plugin and when your documentation changes are done let's close this and open a new pull request for 1.2.3-wip <- amd-plugin. Thanks.

Member

markhealey commented Aug 28, 2013

@montlebalm I'd like to merge this into a 1.2.3-wip branch (which hasn't been created yet) instead of master since this is likely to be launched with some other updates. Keep working in amd-plugin and when your documentation changes are done let's close this and open a new pull request for 1.2.3-wip <- amd-plugin. Thanks.

@markhealey

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Aug 29, 2013

Member

After discussions with @ilinkuo and @rwadkins yesterday, I think we should hold off on a pull request and iterate in this amd-plugin branch until we get it right.

Member

markhealey commented Aug 29, 2013

After discussions with @ilinkuo and @rwadkins yesterday, I think we should hold off on a pull request and iterate in this amd-plugin branch until we get it right.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Aug 30, 2013

@markhealey, @montlebalm Thanks for holding off on this. I'm going to go over this over this weekend to see if there's anything we'd like to change. I think that in the future, for a major feature branch like this, it would be good to announce it on the google groups, or on a F2Dev twitter channel. For example, something like

Chris is starting work on introducing AMD into F2. Please check out the branch <link> and offer your feedback!

twitted or announced via Google Groups on 8/22, the day that Chris started, would have been good. This singles out this event as something worth noting over all the other Github events.

ilinkuo commented Aug 30, 2013

@markhealey, @montlebalm Thanks for holding off on this. I'm going to go over this over this weekend to see if there's anything we'd like to change. I think that in the future, for a major feature branch like this, it would be good to announce it on the google groups, or on a F2Dev twitter channel. For example, something like

Chris is starting work on introducing AMD into F2. Please check out the branch <link> and offer your feedback!

twitted or announced via Google Groups on 8/22, the day that Chris started, would have been good. This singles out this event as something worth noting over all the other Github events.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@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

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.

Show comment
Hide comment
@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

Member

markhealey replied Aug 30, 2013

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.

Show comment
Hide comment
@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.

Member

montlebalm replied Aug 30, 2013

"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.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@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 "_"?

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.

Show comment
Hide comment
@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.

Contributor

Raylehnhoff replied Aug 30, 2013

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.

Show comment
Hide comment
@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.

Member

montlebalm replied Aug 30, 2013

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.

Show comment
Hide comment
@markhealey

markhealey Aug 30, 2013

Member

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

Member

markhealey replied Aug 30, 2013

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

This comment has been minimized.

Show comment
Hide comment
@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.

Member

montlebalm replied Aug 30, 2013

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.

Show comment
Hide comment
@markhealey

markhealey Aug 30, 2013

Member
Member

markhealey replied Aug 30, 2013

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@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);
}

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.

Show comment
Hide comment
@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.

Member

montlebalm replied Aug 30, 2013

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.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@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.

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

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@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?

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.

Show comment
Hide comment
@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.

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.

Show comment
Hide comment
@ilinkuo

ilinkuo Aug 30, 2013

The reqparam doesn't seem to be used?

The reqparam doesn't seem to be used?

This comment has been minimized.

Show comment
Hide comment
@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
});
Member

montlebalm replied Aug 30, 2013

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.

Show comment
Hide comment
@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.

Member

montlebalm replied Aug 30, 2013

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.

Show comment
Hide comment
@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.

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.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Aug 31, 2013

Useful trick -- ignore whitespace in github diffs by adding ?w=1. See https://github.com/blog/967-github-secrets.
You won't be able to comment in the ignore whitespace view. however. You have to go back to the original diff to make comments.

ilinkuo commented Aug 31, 2013

Useful trick -- ignore whitespace in github diffs by adding ?w=1. See https://github.com/blog/967-github-secrets.
You won't be able to comment in the ignore whitespace view. however. You have to go back to the original diff to make comments.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Aug 31, 2013

RequireJS documentation indicates onload.error(e) is correct?

RequireJS documentation indicates onload.error(e) is correct?

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 1, 2013

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.
    #1 and #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.

ilinkuo commented on e2dfa28 Sep 1, 2013

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.
    #1 and #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.

This comment has been minimized.

Show comment
Hide comment
@montlebalm

montlebalm Sep 2, 2013

Member

@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.

Member

montlebalm replied Sep 2, 2013

@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.

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 3, 2013

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.

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.

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 3, 2013

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.

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.

This comment has been minimized.

Show comment
Hide comment
@montlebalm

montlebalm Sep 3, 2013

Member

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.

Member

montlebalm replied Sep 3, 2013

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.

This comment has been minimized.

Show comment
Hide comment
@brianbaker

brianbaker Sep 3, 2013

Member

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.

Member

brianbaker replied Sep 3, 2013

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.

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Sep 3, 2013

Member

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.
Member

markhealey replied Sep 3, 2013

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.

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 4, 2013

@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.

@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.

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 4, 2013

@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/

@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/

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 4, 2013

@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.

@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.

@markhealey

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Sep 3, 2013

Member

PS - I didn't realize GitHub.com copied comments made on a specific commit into the pull request tracking those commits. This site never ceases to impress me—collaboration at its best.

Member

markhealey commented Sep 3, 2013

PS - I didn't realize GitHub.com copied comments made on a specific commit into the pull request tracking those commits. This site never ceases to impress me—collaboration at its best.

@markhealey

This comment has been minimized.

Show comment
Hide comment
@rwadkins

This comment has been minimized.

Show comment
Hide comment
@rwadkins

rwadkins Sep 4, 2013

I'm trying to get up to speed on this conversation, but I'm missing a bit of background and a failure to understand the real problem statement. @ilinkuo, is the crux of your issue with this solution is that it's too far removed from an AMD solution to be considered useful in an AMD environment? Is it possible to highlight the feature gaps of the current loader, this solution, and a "true" AMD solution? Then consider what would be missing in an AMD solution that the current loader provides?

I hate it when latecomers slow down a conversation ;)

rwadkins commented Sep 4, 2013

I'm trying to get up to speed on this conversation, but I'm missing a bit of background and a failure to understand the real problem statement. @ilinkuo, is the crux of your issue with this solution is that it's too far removed from an AMD solution to be considered useful in an AMD environment? Is it possible to highlight the feature gaps of the current loader, this solution, and a "true" AMD solution? Then consider what would be missing in an AMD solution that the current loader provides?

I hate it when latecomers slow down a conversation ;)

@markhealey

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Sep 4, 2013

Member

Not at all, @rwadkins. Good thoughts.

Member

markhealey commented Sep 4, 2013

Not at all, @rwadkins. Good thoughts.

@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 5, 2013

@rwadkins

I'll summarize my position. Let me know if you have any other questions

The pull request under discussion provides an easier way to load F2 Apps by implementing a RequireJS/AMD plugin. However, it is my opinion that

  • the functionality it provides is in no way dependent upon AMD
  • it provides none of the advantages of AMD
    • the cached value provided is useless to the user
    • there is no reduction in redundant resource loading
  • it behaves in a non-idempotent manner inconsistent with every other AMD plugin

Thus, it would be better to leave AMD out of this entirely and rewrite the functionality as a new method F2.easyLoad() which could be useful to both AMD and non-AMD users.

In the course of discussion, @montlebalm proposed an alternate form of the proposed plugin API. Both @markhealey and I preferred this form. I recommended that this alternate form be used, ASAP. However, as there is no code yet written for the alternate form, MOD would like to move forward by modifying the existing pull request, though they haven't said whether they intend to change the API. If the revised pull request still uses the original proposed API, then I am opposed, because there is no way to smoothly iterate from one API to the other.

ilinkuo commented Sep 5, 2013

@rwadkins

I'll summarize my position. Let me know if you have any other questions

The pull request under discussion provides an easier way to load F2 Apps by implementing a RequireJS/AMD plugin. However, it is my opinion that

  • the functionality it provides is in no way dependent upon AMD
  • it provides none of the advantages of AMD
    • the cached value provided is useless to the user
    • there is no reduction in redundant resource loading
  • it behaves in a non-idempotent manner inconsistent with every other AMD plugin

Thus, it would be better to leave AMD out of this entirely and rewrite the functionality as a new method F2.easyLoad() which could be useful to both AMD and non-AMD users.

In the course of discussion, @montlebalm proposed an alternate form of the proposed plugin API. Both @markhealey and I preferred this form. I recommended that this alternate form be used, ASAP. However, as there is no code yet written for the alternate form, MOD would like to move forward by modifying the existing pull request, though they haven't said whether they intend to change the API. If the revised pull request still uses the original proposed API, then I am opposed, because there is no way to smoothly iterate from one API to the other.

@OpenF2 OpenF2 locked and limited conversation to collaborators Dec 9, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.