Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl does not support single string (non-array) for dependencies #8

Closed
asilvas opened this issue Aug 17, 2011 · 9 comments
Closed

curl does not support single string (non-array) for dependencies #8

asilvas opened this issue Aug 17, 2011 · 9 comments
Assignees
Milestone

Comments

@asilvas
Copy link
Contributor

asilvas commented Aug 17, 2011

Requiring "curl(['module1'])" instead of "curl('module1')" is inconsistent with the specifications for AMD, see https://github.com/amdjs/amdjs-api/wiki/AMD

This actually caused me to have difficulty in initially utilizing the library as I was using single dependencies, not realizing that the array was currently required.

@unscriptable
Copy link
Member

Hey @asilvas, the AMD spec documentation may be confusing in this regard. The global library function to kick-off the loading process and the special dependency called "require" used internal to modules are not the same function.

Unfortunately, since RequireJS names these two functions the same has caused lots of confusion. James has recently taken steps to fix this ambiguity by adding a new global, "requirejs": requirejs/requirejs@be45948

I am not sure about the latest version of RequireJS, but at the global level its require('module1') used to behave the same as curl('module1') (as of RequireJS 0.2) which is the same as the special "require" dependency:

define(['require', 'depA'], function (require) {
    var depA = require('depA'); // "require" as an RValue for node/commonjs compat
});

This is for node.js/CommonJS Modules 1.x compatibility when used with a node-to-AMD module converter. Note the 'depA' in the dependency list. The converter would add that when it wrapped the module in the define(). James recently added support for require-as-an-RValue detection on-the-fly and/or automatic wrapping in the build/optimizer tool. This is cool and is something I am working on.

Let me know if I answered your question satisfactorily. :)

-- John

@ghost ghost assigned unscriptable Aug 18, 2011
@unscriptable
Copy link
Member

Closing this unless you'd like to chat more about it. :)

@asilvas
Copy link
Contributor Author

asilvas commented Aug 25, 2011

Not a big deal, but my vote still stands.

curl("module1");
curl("module2");

This (optional) syntax works really well for apps with a lot of dependencies scattered around.

@unscriptable
Copy link
Member

The main curl() entry point is a bit overloaded already. I could consider curl.get() or something like that. hmmm....

Just to be sure, I think you're referring to fetching dependencies at a global level, but if you are referring to loading dependencies within an AMD module, then you should probably be using the "local require":

define("myModule", ["require", "depA"], function (require) {

    var a = require("depA"); // node-like syntax, aka "require as an RValue"

    require(["someOtherDep"], function (someOtherDep) {
        // do something with someOtherDep
    });

});

curl 0.5.x supports the "require as an RValue" syntax only if the module can be guaranteed to already be loaded. One way to do this is to put it in the dependency list. This, of course, seems silly if you're writing the define() by hand rather than generating a define() wrapper for node.js modules.

RequireJS can scan the modules before executing them so it can ensure that any dependencies using the RValue syntax are pre-loaded. This eats up CPU cycles and doesn't work on Blackberry browsers, but can be convenient for some use cases. This is something that 0.6 will be able to do, too. (I know, I know. I am teasing you about 0.6 again. :) )

@asilvas
Copy link
Contributor Author

asilvas commented Aug 26, 2011

Perhaps I'm not understanding part of how curl currently works. Curl and "define" are both separate functions, correct? (Not the same function with multiple pointers). I wasn't talking about changing the define call. Only to allow single module loads on a given curl call.

Currently you can:

curl(["module1"]);

But in this case, I only have one dependency, so I'd like to optionally be able to also use:

curl("module1");

I understand it is not currently supported, and that you already use overloading (object indicates config, array for dependencies, and function for callback), so unless I am missing something a "string" could be easily translated into an array of one dependency. Of course if this causes conflicts or bloat that I am not seeing, I understand, just a suggestion.

@unscriptable unscriptable reopened this Aug 27, 2011
@unscriptable
Copy link
Member

I understand it is not currently supported, and that you already use overloading (object indicates config, array for dependencies, and function for callback), so unless I am missing something a "string" could be easily translated into an array of one dependency. Of course if this causes conflicts or bloat that I am not seeing, I understand, just a suggestion.

Some of the other AMD loader authors are in favor of removing one of the overloads. Specifically, they want to no longer support a first parameter as a config object. Instead, config would happen one of two ways:

// loader (in this case, curl) just looks for a global, e.g. "curlConfig" 
// curl would use the following config automatically
var curlConfig = { /* ... */ };
// explicit call to a config API:
curl.config({ /* config object */ });
curl(deps, init);

I like that this (the second snippet) is more explicit. It also makes it less confusing to have the string overload.

Removing the configuration overload to a config function would break existing apps, so this would have to be a 0.6 feature. However, I could add the string overload now. No need to wait, I guess.

@asilvas
Copy link
Contributor Author

asilvas commented Aug 28, 2011

I think that's a solid idea. Sometimes breaks are unavoidable. At least in this case, it's a justified and intentional one.

@unscriptable
Copy link
Member

The latest dev branch now has this feature. curl(string); Just fyi: It is not documented and probably won't be until the curl.config() feature is working.

@asilvas
Copy link
Contributor Author

asilvas commented Aug 30, 2011

Great!

@asilvas asilvas closed this as completed Aug 30, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants