Skip to content

Component support #451

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

Merged

Conversation

jasonkuhrt
Copy link
Contributor

This adds support for component i.e.:

var foo = angular.module('foo', [
  require('angular-ui-router'),
  ...

I've updated:

  • grunt concat's "banner" to include code to support commonjs.
  • grunt prerelease to ensure component.version is synced up.
  • readme.md to include component instructions.

Let me know how to improve this or if its good to go. Note that I have not cut an actual release which appears to be something inappropriate to do myself.

Thanks.

@jasonkuhrt
Copy link
Contributor Author

No rush, but I have a certain fear that the angular community only wants to support bower, @nateabele can you hint at whether you'll generally support component or not?

@nateabele
Copy link
Contributor

@jasonkuhrt Regarding the general Angular community, it seems so, yes. Personally I'm pretty ambivalent to Bower, and find the setup to be more trouble than it's worth. My hesitancy at this particular PR has more to do with the fact that it rewrites half the readme.

Also, squash your commits.

@jasonkuhrt
Copy link
Contributor Author

@nateabele I am already aware I should squash my commits, but assumed discussion would be needed, and it is.

rewrites half the readme

Sorry, but that's not true. I've literally only added 4 new lines of text:

https://github.com/littlebitselectronics/ui-router/tree/component-support#get-started

You'll see that I tried two versions (and several more uncommitted). Its not easy. My initial attempt was to make component instructions part of your existing 3-step-process but that doesn't work at all because component doesn't share the same steps. The sane path was to add component as its own entry.

Please provide feedback regarding how I can improve the readme, I am more than happy to do so.

@laurelnaiad
Copy link

"(the package exports the angular module name)"...

[and thus you can set up the dependency by doing require('angular-ui-router') within the angular module declaration.]

I understand why it's possible to do this in cases where a module/component/widget has already put itself in the global scope.

Is this a standard practice for "component" components? In other words, would component users be familiar with this technique? (I use RequireJS quite a bit and haven't seen that done, even though it's possible.)

I guess if you're going to export the module name, one could argue that ui-router might as well detect AMD and return the module name in the same manner.

Thanks.

@nateabele
Copy link
Contributor

Sorry, but that's not true. I've literally only added 4 new lines of text:

Sorry, it's tough to assess PRs multiple times a day across multiple projects. All I saw was a big wall of red and green across the /files tab.

You'll see that I tried two versions (and several more uncommitted). Its not easy.

It's easier if you use GitHub's online editor. It lets you flip between code & preview.

Please provide feedback regarding how I can improve the readme, I am more than happy to do so.

Here, I fixed it. Let me know if there any details missing.

diff --git a/README.md b/README.md
index bf9d2d1..54528f5 100644
--- a/README.md
+++ b/README.md
@@ -29,11 +29,12 @@ to change. Using it in a project that requires guaranteed stability is not recom
 **(1)** Get UI-Router in one of 3 ways:
  - clone & [build](#developing) this repository
  - [download the release](http://angular-ui.github.io/ui-router/release/angular-ui-router.js) (or [minified](http://angular-ui.github.io/ui-router/release/angular-ui-router.min.js))
- - or via **[Bower](http://bower.io/)**: by running `$ bower install angular-ui-router` from your console
+ - via **[Bower](http://bower.io/)**: by running `$ bower install angular-ui-router` from your console
+ - or via **[Component](https://github.com/component/component)** by running `$ component install angular-ui/ui-router` from your console

 **(2)** Include `angular-ui-router.js` (or `angular-ui-router.min.js`) in your `index.html`, after including Angular itself

-**(3)** Add `'ui.router'` to your main module's list of dependencies
+**(3)** Add `'ui.router'` to your main module's list of dependencies (if you're using Component, replace `'ui.router'` with `require('angular-ui-router')`)

 When you're done, your setup should look similar to the following:

@@ -46,6 +47,8 @@ When you're done, your setup should look similar to the following:
     <script src="js/angular-ui-router.min.js"></script>
     <script>
         var myApp = angular.module('myApp', ['ui.router']);
+        // For Component users, it should look like this:
+        // var myApp = angular.module('myApp', [require('angular-ui-router')]);
     </script>
     ...
 </head>

@jasonkuhrt
Copy link
Contributor Author

@nateabele Thanks, that looks totally acceptable.

@stu-salsbury Hey, I'm not sure I accurately follow your line of thought.

Is this a standard practice for "component" components? In other words, would component users be familiar with this technique?

We should not conflate component users and component-angular users. Angular has the unique notion of angular.module which warrants a special way to think about component-angular components.

When require('angular-ui-router') is invoked two things happen (like in node). The angular module name (ui.router) is returned and the contents of the requireed module are executed (in this case angular-ui-router registers itself as an angular.module).

With all this mind, I'm not sure what a more predictable pattern would be for integrating componentjs with angular. For instance I do not see the value in a pattern like this (also, this would require more radical changes to angular-ui-router source code):

// an app instance is passed to the component-angular-x
// which is then used to add directives/services/etc
require('angular.ui.router')(someAppInstance)

The best case with least effort that I can think of is to have every component-angular-x register itself with angular and export its module name. Some others came to the same conclusion:

https://gist.github.com/Enome/6194699
https://github.com/component/component/wiki/Components#angularjs

I'd be happy to hear others' thoughts on this of course: @ianstormtaylor @Enome @hugojosefson

I guess if you're going to export the module name, one could argue that ui-router might as well detect AMD and return the module name in the same manner.

Sure? Sounds good to me.

@laurelnaiad
Copy link

I guess I was just thinking that exporting the module name in order to use it in the module dependencies list looked a little bit like voodoo from an angular perspective.

I've been known to use voodoo myself, so not a slight -- I was just wondering whether it was common practice when you're dealing with a component that intends to register itself in the global context (or in the case of angular, as a module) to export the name at which the component can be located (in angular's case as a module name, in window's case as a window property name).

@jasonkuhrt
Copy link
Contributor Author

looked a little bit like voodoo from an angular perspective.

@stu-salsbury Understandable. FYI this is also another perfectly doable and okay pattern:

require('angular-ui-router');

angular.module('foo', [
  'ui.router',
  ...
]);

@laurelnaiad
Copy link

Of course -- I actually find the voodoo attractive and hoped to hear that it was standard practice :)

@jasonkuhrt
Copy link
Contributor Author

@stu-salsbury Still early days in terms of angular + component so hard to say what is standard. I would vote for this pattern as what else can you usefully return?

Its helpful that this pattern still lets you use non-main module modules, i.e. look at the other things you could use from angular-ui-router: https://github.com/angular-ui/ui-router/blob/master/release/angular-ui-router.js#L79-L83.

So this is fully supported:

require('angular-ui-router');

angular.module('foo', [
  'ui.router.compat',
  ...
]);

@nateabele
Copy link
Contributor

@jasonkuhrt Sounds good to me. Ping me when you get the readme & commits cleaned up and I'll get this merged. Also, let me know what I need to do to register a component module.

@jasonkuhrt
Copy link
Contributor Author

@nateabele @stu-salsbury Should we incorporate AMD support or leave that for another issue?

@laurelnaiad
Copy link

From my perspective amd support isnt necessary bc it works fine with requirejs shim.

@jasonkuhrt
Copy link
Contributor Author

@stu-salsbury Thanks for chiming in, I have no valuable opinion here. Then lets not push anything at least within this issue. @nateabele thanks, I will tidy things up and ping you.

@nateabele
Copy link
Contributor

@jasonkuhrt The Gruntfile changes? Fine with me, as long as they don't impact anything else.

@jasonkuhrt
Copy link
Contributor Author

@nateabele Hey, yeah for two things:

  1. checking component.version in component.json like you are doing with bower.json
  2. Adding the necessary if (...) module.exports = ... logic.

1 is innocuous but 2 should be double-checked by someone else to ensure that I am no introducing unexpected results in minification/concatenation edge-cases. @stu-salsbury @nateabele Mind reviewing this?: littlebitselectronics@c5a724f#L0R28

I expect it to end up like this:

/**
 * State-based routing for AngularJS
 * @version v0.2.0
 * @link http://angular-ui.github.com/
 * @license MIT License, http://www.opensource.org/licenses/MIT
 */

 /* commonjs package manager support (eg componentjs) */
 if (module && exports && module.exports === exports){
   module.exports = 'ui.router';
 }

(function (window, angular, undefined) {
/**
 * State-based routing for AngularJS
 * @version v0.2.0
 * @link http://angular-ui.github.com/
 * @license MIT License, http://www.opensource.org/licenses/MIT
 */
 module&&exports&&module.exports===exports&&(module.exports="ui.router"),(function(r,t,e){ ...

@jasonkuhrt
Copy link
Contributor Author

@nateabele I git squashed but I'm not sure how to interpret the result. Is this what you wanted or not, I can't tell.

@nateabele
Copy link
Contributor

The result should be that the changes are all the same, but all in one commit. You should then have to force-push that commit to your remote branch.

@jasonkuhrt
Copy link
Contributor Author

@nateabele Done, thanks for the tip.

nateabele added a commit that referenced this pull request Sep 22, 2013
@nateabele nateabele merged commit b7b2ceb into angular-ui:master Sep 22, 2013
@jasonkuhrt jasonkuhrt deleted the component-support branch September 22, 2013 17:29
@jasonkuhrt
Copy link
Contributor Author

@nateabele Just a note that until a release build is done the component support won't work since the module.exports code isn't in the release/ folder files.

@nateabele
Copy link
Contributor

Yup, I'm aware. We'll probably be doing a release sometime this week. Also, what do I need to do to get this thing listed in Component's registry?

@jasonkuhrt
Copy link
Contributor Author

We'll probably be doing a release sometime this week.

Great

Also, what do I need to do to get this thing listed in Component's registry?

Add your project to this wiki: https://github.com/component/component/wiki/Components#angularjs. I'm assuming you'd add it under the angularjs section.

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

Successfully merging this pull request may close these issues.

3 participants