amdjs / amdjs-api

Houses the Asynchronous Module Definition API
https://groups.google.com/group/amd-implement
4.31k stars 499 forks source link

Dynamic loader plugins + CJS wrapper syntax #24

Closed csnover closed 9 years ago

csnover commented 9 years ago

If the plugin has a dynamic property set to true, then it means the loader MUST NOT cache the value of a normalized plugin dependency, but instead call the plugin's load method for each instance of a plugin dependency.

How does this work in the case where you are using CJS wrapper syntax?:

define(function (require) {
  var foo = require('dynamic-plugin!foo');
});

With the spec as written, this would be valid:

  1. Loader runs RegExp against the factory function to collect dependencies
  2. Loader loads 'dynamic-plugin!foo', but discards the result because the plugin is dynamic
  3. Loader executes the factory function
  4. Synchronous call to require('dynamic-plugin!foo') fails because the result was not cached

I would suggest an update to the spec stating that the value of the dependency should be cached until after the factory function holding the dependency has executed.

jrburke commented 9 years ago

I believe the "dynamic" property should be removed from the API. I have not found a way to reliably support it, and I do not see it working in ES6 either. Maybe an alternate solution for a per-module differing value would be to use map config to map to different loader plugin IDs that have different resource ID segments.

It was added originally I think based on Rawld's motivations to try to support it. However, the requirejs set of loaders do not support it, and based on the listserv discussion about it, it seemed difficult for others to support too:

https://groups.google.com/forum/#!topic/amd-implement/ASREn674nSk

jrburke commented 9 years ago

All that said, if you want to prep a pull request for that dynamic property text update, feel free to do so and I can merge it, but it would be good to get more feedback from loaders that do support it.

csnover commented 9 years ago

OK. I am OK with removing it as well. If features can’t be migrated to ES6 modules then they should not exist.

jakobo commented 9 years ago

+1. AMD should be evolving in the direction of ES6 modules. @jrburke do you have a list of reference for which loaders support dynamic plugins? (Inject does not)

jrburke commented 9 years ago

I do not. As far as I know, only @rcgill in dojo's loader, and @unscriptable in cujojs expressed interested in the dynamic option, so would be good to hear from them.

unscriptable commented 9 years ago

Only curl.js's deprecated js! plugin uses the dynamic option. -- J

On Fri, Dec 12, 2014 at 2:17 PM, James Burke notifications@github.com wrote:

I do not. As far as I know, only @rcgill https://github.com/rcgill in dojo's loader, and @unscriptable https://github.com/unscriptable in cujojs expressed interested in the dynamic option, so would be good to hear from them.

— Reply to this email directly or view it on GitHub https://github.com/amdjs/amdjs-api/issues/24#issuecomment-66820626.

┏( ^◡^)┛ ┗(^◡^ )┓ I rock RaveJS! ┏( ^◡^)┛ ┗(^◡^ )┓

csnover commented 9 years ago

@rcgill isn’t currently an active contributor, I am currently the primary person managing the Dojo loader. It seems like there is a consensus among loader authors to remove the dynamic option. Is this something where I should create a pull request with the change?

rcgill commented 9 years ago

First, a little history. When we (mostly James, John, and myself) negotiated the plugin spec, I argued that the loader should not provide any caching of plugin results--which is basically the dynamic option. My arguments against any plugin result caching in the loader were as follows:

  1. It adds more logic/code to the loader, thereby making the loader bigger and more complicated. And the price is often paid (depending upon the design of the loader) whether or not an application actually uses any plugins. Note that there is probably never a need for plugins in a properly designed application after an appropriate “build” process. Further note that the plugin normalize function is consequent to the decision to have the loader cache plugin module values. The bottom line to this point is that if we take away loader caching of plugins, the loader’s plugin logic reduces to almost nothing.
  2. The plugin "understands" its semantics and therefore has the opportunity for optimizations that the loader does not. In particular, the plugin may be able to use its special knowledge to improve so-called "built" versions of code. An example (which applies to both unbuilt/built applications) is plugin-loaded data that includes references. Consider JSON schema: assuming the plugin returns fully resolved schema definitions, then each referenced definition must be instantiated and kept in the loader’s cache. These copies are not required if the plugin manages its own cache. Of course it can be argued that this extra space requirement is unimportant. But it cannot be argued that we as loader designers understand every use case, and we ought not to try to “fix” things we don’t understand.
  3. If simple caching is all that's required, then this takes about 3 lines in the plugin: load(id, require, cb){ if(cache[id]){ cb(cache[id]) } // else, load the plugin module... }

At the time, these arguments were convincing enough to include the dynamic option. In hindsight, nobody is happy. I am unhappy about the extra logic caused by caching and the normalize functionality, which inclusion defeats optimizations possible with non-caching designs. In my own work done for private clients, when and where it’s important/required, I provide optimized AMD-based systems where the loader all but disappears. I suspect others do the same. For the wider community, it seems these kinds of systems are not valuable. If this is the case, then it seems reasonable to dispose of them.

@csnover: there were some historical reasons that the dojo/text plugin needed to be dynamic…primarily that some very large clients had code and builds that used other techniques for loading text. I you decide to go forward and remove this from the dojo loader, you might want to query those clients first. Feel free to email me with questions and I’ll try to help where I can.

jrburke commented 9 years ago

Given the lack of use, I am fine if @csnover does a pull request to remove the sections, I can merge. It will also be more in line with what I believe will be in ES6, where the loader will track module export objects, and provide hooks for a loader plugin type of system to participate in different parts of the loader pipeline.