Reactive-Extensions / rx.angular.js

AngularJS Bindings for RxJS
Other
827 stars 89 forks source link

Letter casing conventions #121

Open ariutta opened 8 years ago

ariutta commented 8 years ago

Hello,

Minor question -- why do the examples for this repo use lower case rx when the examples for RxJS and RxNode use capitals (Rx and RxNode)? For the RxJS ecosystem, is there a convention on when to prefer rx vs. Rx?

Thanks!

tihuan commented 8 years ago

I only started using rx.angular.js recently. But what I noticed is that injector for rx.angular.js is rx instead of Rx (since Rx is already used by RxJS), which is probably why you see examples in rx instead of Rx.

Depends on which object you want to call, rx for rx.angular.js and Rx for RxJS. But I feel rx wraps Rx anyway, so I personally call rx instead of Rx all the time to keep it consistent.

Hope this helps!

ariutta commented 8 years ago

Thanks for the response, @tihuan. I'm not sure I understand the value of making both rx.angular.js and plain RxJS available inside my Angular code. Since rx.angular.js provides all the functionality of plain RxJS, I shouldn't need to call plain RxJS inside a controller, factory, etc. So wouldn't it be simpler and more consistent with other RxJS examples to just use Rx for the injected rx.angular.js? Is there an Angular convention that requires using lowercase rx? I'm not aware of one.

Using Rx and additionally following a popular Angular style guide would update the README example to look like this:

angular
    .module('example')
    .controller('AppCtrl', AppCtrl);

AppCtrl.$inject = ['$scope', '$http', 'rx'];

function AppCtrl($scope, $http, Rx) {
    function searchWikipedia (term) {
      return Rx.Observable
        .fromPromise($http({
          url: "http://en.wikipedia.org/w/api.php?&callback=JSON_CALLBACK",
          method: "jsonp",
          params: {
            action: "opensearch",
            search: term,
            format: "json"
          }
        }))
        .map(function(response){ return response.data[1]; });             
    }

    $scope.search = '';
    $scope.results = [];

    /*
      Creates a "click" function which is an observable sequence instead of just a function.
    */
    $scope.$createObservableFunction('click')
      .map(function () { return $scope.search; })
      .flatMapLatest(searchWikipedia)
      .subscribe(function(results) {
        $scope.results = results;
      });
}
tihuan commented 8 years ago

Hey @ariutta, thanks for the detailed explanation! I see what you mean now :santa:

Do you think it'd be even more consistent if rx.angular.js injector became Rx instead of rx, so we get:


AppCtrl.$inject = ['$scope', '$http', 'Rx'];

function AppCtrl($scope, $http, Rx) { ... }

This way rx.angular.js overwrites plain Rx and completely eliminates the need of rx?

ariutta commented 8 years ago

Hmm... I believe the injected Rx/rx would be the same as global Rx regardless:

AppCtrl.$inject = ['$scope', '$http', 'rx'];
function AppCtrl($scope, $http, rx) {
  rx === window.Rx
}

vs. (warning: the following would not work with Implicit Annotation)

AppCtrl.$inject = ['$scope', '$http', 'rx'];
function AppCtrl($scope, $http, Rx) {
  Rx === window.Rx
}

vs. (note: the following would require changing rx.angular.js)

AppCtrl.$inject = ['$scope', '$http', 'Rx'];
function AppCtrl($scope, $http, Rx) {
  Rx === window.Rx
}

See also the JS from this plunkr vs. this one.

Maybe the current code uses lowercase rx for the module and factory names because Angular naming conventions call for the first letter to be lowercase for modules and factories.

tihuan commented 8 years ago

Ooh, that's why! What bothered me was having rx in AppCtrl.$inject = ['$scope', '$http', 'rx']; but use Rx in function AppCtrl($scope, $http, Rx) { ... } I feel it's inconsistent.

But if lowerCamelCase is the convention of Angular, then I guess there's no chance that we can use the third approach :/ (using AppCtrl.$inject = ['$scope', '$http', 'Rx'];

Thanks so much for including Plunkr examples for this, btw. Super helpful! :+1: