add firebase requirement to CommonJS support #625
Conversation
Before this commit, using browserify, in order to use angularfire the programmer must do:
```bash
$ npm install --save firebase angularfire
```
```javascript
require('firebase');
require('angularfire');
```
With this commit applied, the programmer can drop the direct dependence with firebase:
```bash
$ npm install --save angularfire
```
```javascript
require('angularfire');
```
No more dependences or worries are added.
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! On 6 June 2015 at 10:53, googlebot [email protected] wrote:
Dr. David Rodenas |
|
This seems reasonable to me although I will defer to our resident browserify expert @bendrucker on if this is kosher. Ben, what are your thoughts? |
|
As far as I know there are two potentially risks (but I'm not sure): If it is accepted: If anyone is installing and requiring manually firebase, it is possible that it becomes duplicated in the JS. If it is not accepted: firebase and angularfire are not forced to match, so, it can be broken with an update of any of both libraries. I'm not sure, but another possibility could be use a peer-dependencies, but, because I do not have tested it I didn't put it here. |
|
Definitely against this. The original code snippet makes the wrong assumption and I don't see what's gained by this.
We don't want this because angularfire doesn't depend on The actual code would look like: var Firebase = require('firebase')
var angularfire = require('angularfire')Requiring things without assignments is code smell, unless we're talking about polyfills. Because AngularFire no longer calls
Browserify is smart about that and won't duplicate
A peer dep would be a correct change (but solves an entirely unrelated problem). But since Firebase doesn't tend to ship breaking changes, that's more trouble than it's worth. In theory, v1 and v2 of Firebase could be totally API incompatible according to semver. That means AF would want to specify that it expects to be installed alongside Firebase v2 and should complain if it sees v1. Since those sorts of breaking changes don't happen, I'd skip the peerDep for now and come back to if you ever want to drop support for an old version of the client. |
|
100% agree with @bendrucker The reason of making angularfire to require Firebase is because the user is supposed to work with angularfire and do nothing with Firebase direcly. So, it does not seems very good idea to ask to the user to load a library with sideefects (global variables), a library which is a dependence of the one that the user actually wants to use. Is like to ask people to require lodash to use grunt. (Here I discuss the concept, not the proposed implementation). The problem is that in the current implementation angularfire is currently:
Excerpt from: angularfire/dist/angularfire.js (function(exports) {
"use strict";
// Define the `firebase` module under which all AngularFire
// services will live.
angular.module("firebase", [])
//todo use $window
.value("Firebase", exports.Firebase);
})(window);( https://github.com/firebase/angularfire/blob/v1.1.1/dist/angularfire.js#L12-21 ) Main module of angularfile package is angularfire/index.js which exports a string: ( https://github.com/firebase/angularfire/blob/v1.1.1/index.js ) This pattern is the same (or almost the same) adopted by angular in the oficial packages:
It does not mean that it is ok, but it is probably the inspiration of current sources. Probably the main reason of this schema is to make the same source compatible with bower and npm without adding explicit code. The problem is that means that we have global variables and global state. This situation is expected to be solved in Angular 2. Meanwhile this pullrequest may be a little sweet to ease developments (make them more consistent with browserify angular developments). |
I landed the PRs to do this on Angular so if you disagree you know who to talk to :) The point of exporting strings is so you can do this: require('angular').module('myFbApp', [require('angularfire')]If I'm understanding right, the concern is that you can't inject Firebase because it's not necessarily on the window yet when the value is created w/in Angular. That's totally valid, but the problem there is that Firebase shouldn't be injectable at all. It's a bad idea to expect users to supply already constructed objects but then also make assumptions about the constructor being globally available. It's a breaking change to rip that out. The answer for Browserify users is to not bother with the dependency injector because you have a better way to get a hold of Firebase ( |
|
I do not disagree, in fact, in my developments I use this formula. And of course, my newest devs packages are 100% npm packages, without assuming global variables. The concern is that I do not need any instance of Firebase, not in the window not in the app. It is angularfire who requires it, but, instead of loading the dependence by itself, it asks you to do it. |
I do not see how that is possible. All major functions of the angularFire library require you to pass in a Firebase reference. Your are not going to be able to provide that without requiring the constructor some where in your code. For example, how does it help with this (modified from the quickstart example): var Firebase = require('firebase');
app.controller("SampleCtrl", function($scope, $firebaseArray) {
// you are going to need this constructor call somewhere!
var ref = new Firebase("https://<YOUR-FIREBASE-APP>.firebaseio.com/messages");
$scope.messages = $firebaseArray(ref);
});@drpicox - If you still feel strongly about this, can you create a minimal sample project that demonstrates how this helps. As of right now, I don't see it. Also, FYI, the decision to export a string goes back to #567 from this repo, and #10732 from angular.js. The decision to do so was given a lot of thought and discussion, so you are fighting a steep uphill battle if you want to change that practice. |
|
Right, so the correct answer is to not register Firebase with the injector at all. Technically AngularFire depends on Firebase in order to make it injectable. The ideal move here is to cut the This is an equally breaking change with equally little benefit. @jwngr my final rec is to close this and cut the |
|
Maybe we should deprecate injection via angular-di immediately, but not break anything until later. (function(exports) {
"use strict";
var log = true;
// Define the `firebase` module under which all AngularFire
// services will live.
angular.module("firebase", [])
//todo use $window
.value("Firebase", function(url) {
if (log) {
log = false; // only log once.
console.log("Injecting the Firebase constructor via angular-di is deprecated. Either access it via the global variable, or require it via CommonJS");
}
return new exports.Firebase(url); // does it need to handle more than one argument?
});
})(window); |
|
Ops, that's right. Anyway, before deprecating it, you should consider that angular DI is far more advanced than npm require (accepts redefinitions, decorators, ...). |
|
AngularFire never actually calls If you are talking redefining / decorating dependencies during testing, then:
I think proxyquire provides a much cleaner API. As for decorating instances for production, angular-di may be your best choice. |
|
So, if I understood well, this: should be a peer-dependence at most, right? |
|
Proxyquire seems pretty neat ;) I wrote proxyquire-universal and maintain proxyquire(ify). They're pretty great and Thorsten deserves all the credit for the API and the implementation.
You are correct (the code isn't). |
|
Thanks for all the discussion here folks. I'm closing this issue per @bendrucker's suggestion. I doubt you will see any changes on this front for AngularFire for Angular 1.x. We are in maintenance mode with this library and will be focusing efforts on AngularFire for Angular 2.x going forward (although that doesn't actually exist yet). We will re-address this topic when that new library comes out. FWIW, having Firebase as a peer dep makes sense and is probably what we should have done. But I agree that these other changes bring little to no benefit. |
The idea of this pull request is to simplify the development of apps using firebase and angularfire with browserify. Instead of requesting the programmer to add two explicit dependences (angularfire and firebase, one direct and another required by the former), it translates to angularfire the responsability to load firebase dependence (the latter).
Before this commit, using browserify, in order to use angularfire the programmer must do:
With this commit applied, the programmer can drop the direct dependence with firebase:
No more dependences or worries are added.