Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add AMD support to Flash event dispatch #111

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants

This adds support for AMD on the Flash side, fixing issue #101. It also adds documentation detailing some of the consequences. Please see issue #101 for more information.

This is a pretty long line. I suggest breaking it up over multiple lines to aid readability.

Owner

chrisbreiding replied Mar 3, 2013

Do you have a preference between these 2 options, or perhaps a better way to break up the long string?

Option 1:

private var wrappedDispatch:String = '(function (event, args) {';
                  wrappedDispatch +=   'require(["ZeroClipboard"], function (ZeroClipboard) {';
                  wrappedDispatch +=     'ZeroClipboard.dispatch(event, args);';
                  wrappedDispatch +=   '});';
                  wrappedDispatch += '})';

Option 2:

private var wrappedDispatch:String = ( <![CDATA[
  (function (event, args) {
    require(["ZeroClipboard"], function (ZeroClipboard) {
      ZeroClipboard.dispatch(event, args);
    });
  })
]]> ).toString();

Then:

private var externalDispatch:String = isAMD() ? wrappedDispatch : 'ZeroClipboard.dispatch';
Owner

chrisbreiding replied Mar 3, 2013

After some testing it looks like option 1 would actually have to be wrapped in a function itself, so the options would be:

Option 1:

private function wrappedDispatch(): String {
  var wrappedDispatch:String = '(function (event, args) {';
            wrappedDispatch +=   'require(["ZeroClipboard"], function (ZeroClipboard) {';
            wrappedDispatch +=     'ZeroClipboard.dispatch(event, args);';
            wrappedDispatch +=   '});';
            wrappedDispatch += '})';
  return wrappedDispatch;
}

private var externalDispatch:String = isAMD() ? wrappedDispatch() : 'ZeroClipboard.dispatch';

Option 2:

private var wrappedDispatch:String = ( <![CDATA[
  (function (event, args) {
    require(["ZeroClipboard"], function (ZeroClipboard) {
      ZeroClipboard.dispatch(event, args);
    });
  })
]]> ).toString();

private var externalDispatch:String = isAMD() ? wrappedDispatch : 'ZeroClipboard.dispatch';

Even though I'm not a huge fan of CDATA sections, I think option #2 looks cleaner.

Owner

JamesMGreene commented Mar 10, 2013

Hey @jrburke @unscriptable, could you guys offer an AMD consult?

Please take a quick peek at this PR and its related issue (#101) and let me know if there are any other options we could explore for AMD users. I'm never wild about forcing module names on consumers but forcing them to tack an object onto the global object is also unfortunate.

Owner

JamesMGreene commented Mar 10, 2013

What about having AMD users set an additional config value (i.e. amdModuleName) on the ZeroClipboard object that would be passed into the Flash object? That way we could optionally avoid the need to check for the existence of AMD from Flash (although your check is very clever!) as we can put that burden on the users. More importantly, the users can keep their module names as whatever they want, so long as their ZeroClipboard module configuration has some name. Then in the ActionScript changes for this PR, we would just update Line 209 to utilize the passed value for amdModuleName instead of hard-coding it to "ZeroClipboard".

Example usage for an AMD user:

require(['ZeroClipboardOrWhatever'], function(ZeroClipboard) {
    ZeroClipboard.setDefaults({
        moviePath: "lib/ZeroClipboard.swf",
        amdModuleName: "ZeroClipboardOrWhatever"
    });
    var clip = new ZeroClipboard(document.getElementById('copy-button'));
    clip.on('load', function () {
        // yay, this works now!
    });
});

cc: @jrburke @unscriptable — Are there any other AMD config options that might work better here that I'm forgetting...?

@unscriptable unscriptable commented on an outdated diff Mar 10, 2013

docs/instructions.md
@@ -461,6 +461,20 @@ Here is a complete example which exercises every option and event handler:
</html>
```
+## AMD
+
+If using AMD, such as RequireJS, note that the SWF file relies on a hard-coded path of 'ZeroClipboard' defined for ZeroClipboard.js. Either put ZeroClipboard.js in the root of your RequireJS base path or configure the path like so:
@unscriptable

unscriptable Mar 10, 2013

It seems this doc section should be included regardless of whether the swf relies on the AMD module's ID or not. This paths config is likely needed unless the ZeroClipboard is placed at baseUrl, irregardless of the swf. Hmmm... I guess some folks would just load the module via its path instead of its ID. Still, I think this section should probably stay in the docs (and not imply that it's needed for the swf) even if we find a way to not hard-code the ID inside the swf.

Also: remove "hard-coded" in the previous sentence. It's confusing, imho, and took me a while to figure out what you were saying. (All config is hard-coded :) .) The "hard-coded" below makes perfect sense to me.

Any interest in mentioning other AMD loaders? :)

@unscriptable unscriptable commented on an outdated diff Mar 10, 2013

src/flash/ZeroClipboard.as
+ // determines if AMD is being used
+ //
+ // returns boolean
+ private function isAMD(): Boolean {
+ return ExternalInterface.call( '(function () { return typeof define === "function" && define.amd; })');
+ }
+
+ // amdWrappedDispatch
+ //
+ // string javascript function used to call ZeroClipboard.dispatch with AMD
+ //
+ // returns string
+ private function amdWrappedDispatch(): String {
+ return ( <![CDATA[
+ (function (event, args) {
+ require(["ZeroClipboard"], function (ZeroClipboard) {
@unscriptable

unscriptable Mar 10, 2013

Two issues here:

  1. Global require. The global require is an optional global in AMD. curl.js, for instance, does not declare it by default (which has eliminated tons of newb confusion). Also: developers can change the name of AMD globals (define, curl, require, etc.).
  2. This is tricky since it is an async call. Isn't there a chance in IE that ZeroClipboard.dispatch() could get called before the async require() returns?

I have no knowledge of how flash/ActionScript and browser/JavaScript interact, but I imagine there's a way to use define instead of require here. Something like this?

// execute this "named module". Note: the name is arbitrary, but should be unique / "namespaced".
define('ZeroClipboard/dispatch-capture', ['ZeroConfig'], function (ZC) { /* is there a way for ActionScript to capture ZC.dispatch here? */ });

This doesn't solve the async issue, but does mostly solve the global naming issue. Devs who want to change the name of define are accustomed to having to change source code, so they won't complain much, I imagine.

Alternatively: could we pass the name of the global define or require into the swf as a parameter???

@unscriptable

unscriptable Mar 10, 2013

If we could use @JamesMGreene's parameter-passing solution and add an additional parameter to name the global loader function, that would work, I believe.

Ah, @JamesMGreene, I saw your suggestion after I commented on the PR. :) I guess my idea of stuffing a parameter into the swf at load time could be also be done using your method.

Btw, I think you mean define instead of require:

define(['ZeroClipboardIDOrFullPath'], function(ZeroClipboard) {
    ZeroClipboard.setDefaults({
        moviePath: "lib/ZeroClipboard.swf",
        amdModuleName: "ZeroClipboardIDOrFullPath",
        amdLoaderName: "curl" // or "requirejs" or "mynamespace.load"
    });
    var clip = new ZeroClipboard(document.getElementById('copy-button'));
    clip.on('load', function () {
        // yay, this works now!
    });
});

From this code snippet, it also seems like the async issue is not a problem. My misunderstanding. Well, that is true if users don't expect the code inside your example "load" handler to execute sync since it will likely execute async.

My latest commits add the ability to configure the AMD path/id and loader function as per the suggestions of @JamesMGreene and @unscriptable. Awesome suggestions, by the way, since they deal with the main drawback of this approach and also add better support for other AMD libraries (I was myopically only considering RequireJS).

This makes amdModuleName a required configuration option if a user is using AMD. By default, the code assumes a global require function, but that can be configured with the amdLoaderName option if curl or another library is being used.

I'm passing the configuration options to the ActionScript through the flashvars. Please advise if there is a better way to do that.

Owner

JamesMGreene commented Mar 12, 2013

Btw, I think you mean define instead of require:

define(['ZeroClipboardIDOrFullPath'], function(ZeroClipboard) {
ZeroClipboard.setDefaults({
moviePath: "lib/ZeroClipboard.swf",
amdModuleName: "ZeroClipboardIDOrFullPath",
amdLoaderName: "curl" // or "requirejs" or "mynamespace.load"
});
var clip = new ZeroClipboard(document.getElementById('copy-button'));
clip.on('load', function () {
// yay, this works now!
});
});
From this code snippet, it also seems like the async issue is not a problem. My misunderstanding. Well, that is true if users don't expect the code inside your example "load" handler to execute sync since it will likely execute async.

@unscriptable Is there any overhead from generating a new anonymous module with define for every callback? That's why I had used the [non-standard] require instead.

Hey @chrisbreiding, thanks for mentioning curl.js! <3 <3 Minor request: can you change it to say "curl.js"? We refer to it that way for better googling and differentiation from the Unix utility. :) Thanks -- John

lgtm! :)

Is there any overhead from generating a new anonymous module with define for every callback?

Hey @JamesMGreene, I don't understand. All AMD modules use a define(). If a dev is using ZeroClipboard in AMD, they'll be doing it inside a module -- in other words, inside a define(). I just changed your code to look like a module.

The way you wrote it (with require()), it is just executing global code in a callback after ensuring ZeroClipboard is loaded. This is not the right way to write AMD. That's more like LABjs. Devs should be thinking and writing modules (whether AMD or not), not global-ish code. :) IMHO

-- John

Owner

JamesMGreene commented Mar 12, 2013

@unscriptable I'm pretty AMD savvy so I think we're not on the same page with regard to execution contexts. I'll post more of an execution flow tonight to try to clarify.

Owner

JamesMGreene commented Mar 13, 2013

AMD Execution Flow for ZeroClipboard

index.html

Import RequireJS (or any other AMD loader) and kick it all off:

<!DOCTYPE html>
<html>
    <head>
        <title>ZeroClipboard AMD Demo</title>
        <!-- data-main attribute tells require.js to load scripts/main.js after require.js loads. -->
        <script data-main="scripts/main" src="scripts/require.js"></script>
    </head>
    <body>
        <h1>Demo, OMG!</h1>
    </body>
</html>

main.js

"main.js" will create some AMD module ("myMainModule") with a dependency on ZeroClipboard (which is exposed as an AMD module if there is an AMD loader present on the page), update the ZC config, create a ZC client instance, and attach an event listener callback:

define('myMainModule', ['lib/zc/ZeroClipboard'], function(ZeroClipboard) {
  ZeroClipboard.setDefaults({
    moviePath: 'lib/zc/ZeroClipboard.swf',
    amdModuleName: 'lib/zc/ZeroClipboard'
  });
  var clip = new ZeroClipboard(document.getElementById('copy-button'));
  clip.on('complete', function(data) {
    // Yay, JS event listener is attached... BUT it's not quite that easy when Flash is involved!
    console.log('Injected into the clipboard: ' + data.text);
  });
});

ZeroClibpoard.swf / ZeroClipboard.as

After the user tries to click on the HTML button layered beneath the associated ZC client instance (a transparent Flash object), the "ZeroClibpoard.swf" will inject developer-configured text data into the user's clipboard. When successfully inserted, the Flash object will then issue an ExternalInterface.call command to tell the JS-side ZC dispatcher (in the non-AMD scenario, the dispatcher is a globally available ZeroClipboard.dispatch method) that the action was successfully completed. Due to the limitations of JS-to-Flash and Flash-to-JS interoperability (and herein lies the original issue), ExternalInterface.call is actually nothing more than a thinly veiled window.eval executed in the global scope... but in an AMD scope, the ZeroClipboard object is not globally available to allow its dispatch method to be invoked via ExternalInterface.call!

This dilemma brings us to where you and I are currently not on the same page [yet]. Now, to make this all hook fluidly together in an AMD application, the Flash object must be able to get in touch with the ZeroClibpoard.dispatch method from the global scope. So, we can either utilize a non-module-creating function like RequireJS's global require but, as you noted, that is not a part of the AMD standard and therefore should be avoided. As the only global function that is required by the AMD standard is define, it seems that we should follow your advice and wrap the script contents of every ExternalInterface.call invocation (originally just a plain ole ZeroClipboard.dispatch) to include that, e.g. ExternalInterface.call (a.k.a. window.eval):

(function(event, args) {
  // Remember: 'lib/zc/ZeroClipboard' == `amdModuleName` FlashVars item from "main.js"
  define(['lib/zc/ZeroClipboard'], function(ZeroClipboard) {
    ZeroClipboard.dispatch(event, args);
  });
})('complete', { text: 'blah' });

This means that, when using AMD, we will be creating a new anonymous module (as seen immediately above) to wrap the event dispatching, which fire every time a user clicks on a ZeroClipboard client object to inject text into their clipboard.

And so, my last question to you (@unscriptable) was:

Is there any overhead from generating a new anonymous module with define for every callback?

Does my question make sense now?

P.S. If we use the global define method, as I think we must to be safe, then we can also drop the amdLoaderName FlashVars item.

Owner

JamesMGreene commented Mar 18, 2013

@unscriptable Can you please respond when you get a chance? Thanks!

@jrburke Would really love feedback and/or other options/ideas from you on how to make this work as well. Thanks!
_~~ James Greene, your JSConf 2012 lunch-time-acquaintance_

Hey @JamesMGreene!

Generating anonymous module on every callback is not a good idea. :) When I said, "Btw, I think you mean define instead of require:", I was referring to your code snippet in this comment which you later corrected in your awesomely detailed later comment.

The solution you and @chrisbreiding have now (configurable loader name with default == "require") looks fine to me!

-- John

Owner

JamesMGreene commented Mar 19, 2013

Posted a message to the RequireJS mailing list in the hopes of getting feedback from @jrburke's RequireJS camp: https://groups.google.com/d/topic/requirejs/FntaEC55JMg/discussion

Owner

JamesMGreene commented Apr 1, 2013

Finally talked to @jrburke (on IRC) and he [unsurprisingly] agrees with @unscriptable's final assessment:

  • Yes, use the global require instead of define.
  • Yes, use a configurable name for AMDjs libs that use a function name other than require, e.g. curl.

Updated Flash callback to emit events

(function(event, args) {
  require(['lib/zc/ZeroClipboard'], function(ZeroClipboard) {
    ZeroClipboard.dispatch(event, args);
  });
})('complete', { text: 'blah' });

Notes:

  • 'lib/zc/ZeroClipboard' == amdModuleName FlashVars item from "main.js"
  • require == amdLoaderName FlashVars item, to be added from "main.js" (as in earlier comments)

Remaining discussion

I would like to come up with a clearer name for the amdLoaderName FlashVars item, though, e.g. amdRequireName, amdRequireFuncName, amdRequireFunctionName, amdGlobalRequireName, amdGlobalRequireFuncName, amdGlobalRequireFunctionName, etc.

The AMDjs spec does refer to this optional function as "the global require function". Although it's technically optional, it's realistically required in one form or another as the function typically serves the role of the AMD loader's global entry point for kicking off the page's module loading chains.

Various AMD loader implementation names:

  • RequireJS: require/requirejs
  • curl.js: curl
  • needs: require
  • lsjs: require/lsjs
  • Inject: require/Inject.require

I'd love to implement this into my Backbone/require project... Any more progress on this?

How about just requireLoader? "name" seems sort of redundant (almost like calling a variable fooVar or varFoo). "Require" might be more recognizable to the masses (who don't care/know about AMD theory) but either way requireLoader or amdLoader is immediately comprehensible, "Oh, that must be the var that holds the loader for the require/amd library". Adding "name" to the end doesn't really get you much more comprehension. "loader" implies it is a function, so, no need to add "Func" in there.

Looking forward to seeing this implemented. :)

@ghost ghost assigned JamesMGreene May 3, 2013

Owner

JamesMGreene commented May 3, 2013

@cmcculloh: I was proposing tacking the "Name" suffix on as I'm worried users who aren't so familiar with the implementation details here might try to pass the require function rather than its global identifier name (e.g. "require"). I'm OK with omitting the "Name" suffix... I'll just add a typeof check to ensure that the value passed in is a string.

Owner

JamesMGreene commented May 3, 2013

I'll try to get this implemented in the next few days.

Definitely tack the 'Name' on there, maybe even 'FunctionName'. I'm one of those not so familiars who would have passed the function. I didn't really understand what the var you were naming was and thought it literally was the function. I thought you were just trying to decide what to call the global function and wanted to call it something other than 'require' so it would be non-conflicting. Sort of like when someone is trying to decide whether to call the jQuery library: $ or jQuery.

So, now that I understand, I'd maybe call it 'amdLibraryFunctionName'.

But, I was more or less just trying to spur conversation/movement and you have invested 1000x more than some random drive by commenter (me) and should do (or not do) what you think is best. :)

Thanks, looking forward to using this.

Sent from my iPhone.

On May 3, 2013, at 1:25 PM, "James M. Greene" notifications@github.com wrote:

@cmcculloh: I was proposing tacking the "Name" suffix on as I'm worried users who aren't so familiar with the implementation details here might try to pass the require function itself rather than its global identifier name. I'm OK leaving the "Name" suffix off... I'll just add a typeof check to ensure that the value passed in is a string.


Reply to this email directly or view it on GitHub.

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 9, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 9, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 9, 2013

@JamesMGreene JamesMGreene referenced this pull request May 9, 2013

Closed

Completing AMD support #145

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 9, 2013

Owner

JamesMGreene commented May 9, 2013

Replaced by PR #145.

PR #144 adds an AMD-based demo page, too.

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 9, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 19, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 21, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 21, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 21, 2013

JamesMGreene added a commit to JamesMGreene/zeroclipboard that referenced this pull request May 22, 2013

Owner

JamesMGreene commented Jul 6, 2013

FYI: as of ZeroClipboard v1.2.0-beta.2, you no longer need to set any special configuration options for AMDJS (or CommonJS) support!

woohoo!

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