cujojs / curl

curl.js is small, fast, extensible module loader that handles AMD, CommonJS Modules/1.1, CSS, HTML/text, and legacy scripts.
https://github.com/cujojs/curl/wiki
Other
1.89k stars 216 forks source link

Pathing issues for plugins in 0.7? #139

Closed asilvas closed 11 years ago

asilvas commented 11 years ago

Took a while to determine what was going on, and while I'm still not certain as to why, I can at least reproduce the simple steps required. Basically it appears the issue is related to specifying a "pluginPath" if you've compiled plugins into a single "curl.js" file. Sometimes we still load plugins dynamically (via our pluginPath) even though most plugins are compiled into curl.js.

curl = {
  pluginPath: "http://someotherlocation/curl/plugin"
}

<script src="../curl/curl.js"></script>

<script>
  curl("js!test.js");
</script>
  1. Since I've specified the "pluginPath", instead of using "curl/plugin/js" already included in "curl.js", it is loading it (again).
  2. If I do nothing more than remove "pluginPath", it will "just work". But then I can no longer load plugins dynamically as well.
  3. Plugin Identifiers seem to have changed? While "define('js')" worked prior, it now seems to require "define('curl/plugin/js')".

While I do not understand #3, it seems this may be the culprit of the issues.

asilvas commented 11 years ago

By the way, our usage of plugins is far more comprehensive than above (including custom ones, which appear to require changes now due to #3), this was just the simplest way I could reproduce it.

unscriptable commented 11 years ago

Hey @asilvas!

You are correct, plugin ids must now be fully-qualified in the built file. This results from a decision that "pluginPath" is an id transform instead of a path/url transform. I'm second-guessing that decision now -- and not just because of the issues it caused you.

I'm going to start a new discussion in the forum. Feel free to join in. https://groups.google.com/d/forum/cujojs

Thanks for your continued feedback, Aaron!

-- John

unscriptable commented 11 years ago

Is there any way you could create a private gist of the path config(s) you have now? I'd really benefit from seeing this.

asilvas commented 11 years ago

It is still not clear to me as to what I need to do to get my plugins working. Is this not a bug?

Our configurations abstract from curl (in an effort to keep options open), so I can't just copy/paste the source. But here is a JSON dump of the live object (minus some internal stuff curl adds after init) after curl.js has loaded; I've also removed most of the module paths as they follow the same pattern.

{"pluginPath":"http://some_domain/starfield/curl/{version}/plugin",
"paths":{
"starfield/jquery.mod":"http://some_domain/starfield/jquery/{version}/jquery.js",
"starfield/sf.core.pkg":"//some_domain/starfield/sf.core/{version}/sf.core",
"starfield/sf.module":"//some_domain/starfield/sf.core/{version}/sf.module",
"starfield/sf.growl":"http://some_domain/starfield/sf.core/{version}/sf.core",
"starfield/sf.tabs":"//some_domain/starfield/sf.tabs/{version}/sf.tabs",
"starfield/sf.dialog":"//some_domain/starfield/sf.dialog/{version}/sf.dialog"
},
"apiName":"require",
"baseUrl":""
}

That is the way it is currently done today. In the next version, modules will no longer have their own version (i.e. "starfield/module/{version}/module" will become "starfield/{version}/module/module"), but the short names will remain unchanged. In fact that's why I need to rewrite the paths, so that "starfield/module" will automatically be rewritten to "//some_domain/starfield/{version}/module/module". This will get rid of the dozens (and ever-increasing) of path definitions we require today.

Let me know if I can provide anything else.

asilvas commented 11 years ago

Had to revert the release at this point, too many issues (not all of which I've been able to provide simple examples of yet). Not being able to specify a plugin path, and the plugin naming are certainly big ones though. My main priority to get curl 0.7 in is for the critical ie10 bug, so I'll continue to do what I can from my end.

unscriptable commented 11 years ago

thanks for the config snippet. this should help me figure out where the tests fall short.

unscriptable commented 11 years ago

Hey @asilvas,

I created a 068-dev branch. This is 0.6.7 + IE10 fixes (and one obscure IE6 fix). Please test it when you get a chance.

This should alleviate any impending emergency while we figure out how to transition your code base to 0.7.x. The changes in 0.7.x are for correctly supporting AMD build tools. You will have to make a few changes, but they shouldn't be major ones.

As you noted, the plugin ids will need to have their prefix information embedded. There may be a flag or two you need to set in the config, too. Some of your other open issues may be due to bugs. I will address each separately.

Thanks again for your support!

-- John

asilvas commented 11 years ago

Much appreciated, John. I'll test out the dev branch soon.

asilvas commented 11 years ago

Sorry, the 068 branch has same bug as Issue #140. That's the only pre-existing issue I've reproduced so far, but is significant since we use "packages" commonly.

ghost commented 11 years ago

I have no idea what's going on in that Google forum, but this seems like a pretty big problem that really warrants some accompanying documentation which details why the define methods now require different arguments and what effects this has on the library as a whole. As it stands right now, I have just upgraded to 0.7, everything is completely broken, and I have absolutely no idea why.

ghost commented 11 years ago

More specifically: If you're going to break the entire plugin API in some way, a line in the README right next to the one encouraging people to upgrade if they want to be compatible with IE10 would be a much better approach to spreading the word about it than just letting everyone discover it on their own and find nothing but a month old issue thread about it with no real solution in place.

unscriptable commented 11 years ago

Hey @khiltd, sorry for the frustration. The undocumented change was obviously a mistake. We're working on new docs now. Are you having issues related to plugins or something else? Can you be more specific or provide some code and/or error snippets?

Also: we've incorporated the IE10 fixes into two additional branches: 0.6.2a and 0.6.8

Hope those help in the mean time.

-- John

unscriptable commented 11 years ago

Plugin ids in compiled files must now be fully specified (e.g. "curl/plugin/css"). Closing issue for now unless you find issues after you've specified the full plugin ids.