angular / angularfire

Angular + Firebase = ❤️
https://firebaseopensource.com/projects/angular/angularfire2
MIT License
7.69k stars 2.19k forks source link

@FirebaseList and @FirebaseObject design #7

Closed jeffbcross closed 8 years ago

jeffbcross commented 8 years ago

Goal: Have a declarative means of getting a Firebase reference into a component, to be used in a view.

Proposal:

@Component({
  selector: 'posts-list',
  changeDetection: ChangeDetectionStrategy. OnPush
  template: `
  <ul>
    <li *ngFor="#post in posts | async">
      {{post.val().title}} 
      <button (click)="posts.remove(post)">X</button>
    </li>
  </ul>
  `
})
class PostsList {
  @FirebaseList({
    path: 'https://<FIREBASE>/posts' // Should eventually support relative to an injectable root.
  }) posts:Observable<Post>;

The @FirebaseList decorator would create a subclassed Observable and set it to posts. The Observable would contain methods for updating the data. It's generally considered bad practice to add non-combinator methods to an Observable, but there was no cleaner alternative that came to mind (I'm hoping for ideas!). Since the type is an Observable it can be unwrapped with the async pipe inside the template, and can take advantage of OnPush change detection to only perform dirty checking when a new array has been emitted from the Observable.

The FirebaseListObservable would support save, add, and remove similar to AngularFire, as well as lookup helpers to find items by key or find index of an item.

This design has the values inside the list as wrapped Firebase objects instead of POJS, which means .val() must be called inside the template. This is to make it easier to update data without having to reverse lookup records. There could be a pipe that would unwrap all objects, or developers could just map themselves:

<li *ngFor="#post in posts | af_unwrap | async">
class PostsList {
  postsUnwrapped:Observable<Post>;
  @FirebaseList(...) posts:Observable<Firebase>;
  constructor() {
    this.postsUnwrapped = this.posts.map(v => v.val());
  }
}

This decorator is called @FirebaseList. There will be a separate decorator with similar semantics but for objects, called @FirebaseObject.

Querying

Query operators may optionally be supplied to the decorator:

@FirebaseList({
  path: 'https://<FIREBASE>/posts',
  query: [['orderByChild', 'timestamp'], ['limitToFirst', 2]]
})

Prior Art https://www.firebase.com/docs/web/libraries/angular/api.html#angularfire-firebasearray

The utilities used to make this functionality would carry over to the yet-to-be-designed Firebase pipe. The pipe would probably be the preferred tool for developers not familiar with, or able to use decorators.

I've got a branch with a functioning FirebaseList decorator that I'll push soon.

davideast commented 8 years ago

These examples looks awesome, Jeff!

I think exposing collections as an Observable will fit well into Angular 2 architecture, and will solve a lot of problems we had in AngularFire 1.

The only other thing I think is needed here is guidance on injecting Firebase references into components.

Just a little question. Is af_unwrap calling .val() on the DataSnapshot?

jeffbcross commented 8 years ago

The only other thing I think is needed here is guidance on injecting Firebase references into components.

Here's the unit test for how I have it implemented locally: (TL;DR: inject DEFAULT_FIREBASE to get the url, or DEFAULT_FIREBASE_REF to get the ref for the default url).

import {Injector, provide} from 'angular2/core';
import {FIREBASE_PROVIDERS, DEFAULT_FIREBASE, DEFAULT_FIREBASE_REF} from './angularfire';

describe('angularfire', () => {
  describe('DEFAULT_FIREBASE_REF', () => {
    it('should provide a FirebaseRef for the DEFAULT_FIREBASE_REF binding', () => {
      var injector = Injector.resolveAndCreate([
        provide(DEFAULT_FIREBASE, {
          useValue: 'https://ng2-forum-demo.firebaseio.com'
        }),
        FIREBASE_PROVIDERS
      ]);
      expect(typeof injector.get(DEFAULT_FIREBASE_REF).on).toBe('function');
    })
  });
});

Just a little question. Is af_unwrap calling .val() on the DataSnapshot?

Yeah, it'd essentially be (naive cased to support array. should also support objects):

class AfUnwrap {
  transform(input:Observable<any[]>):Observable<any[]> {
    return input.map(arr => arr.map(snap => snap.val()));
  }
}

CC @davideast

jeffbcross commented 8 years ago

I pushed my branch in all its WIP glory: https://github.com/jeffbcross/indy/tree/experiments

jeffbcross commented 8 years ago

Another thing I forgot to include in the proposal was how to manage pagination, or making the query more interactive. I want to make it possible to incorporate scalar or observable values into the query part of the list. And when new values would be pushed to an observable, the query would be re-performed underneath.

Since the decorator definition object doesn't have access to the class instance, it wouldn't be possible to include direct references to class properties in the annotation. So there needs to be a way to reference the property by name instead of reference. The two ideas that come to mind are a string-based DSL to specify interpolated properties, i.e. query: [['limitToFirst', '{{pageSize}}']] and a more explicit approach using functions to designate dynamic values: query: [['limitToFirst', observableQuery('pageSize')]]. I lean toward the 2nd, explicit option.

TylerEich commented 8 years ago

What's the advantage of using a @FirebaseList or @FirebaseObject decorator over a service e.g. new Firebase( pathAndOptions ) or getFirebase( pathAndOptions )?

I noticed that Falcor makes extensive use of Observables, but not by subclassing. They use a Model constructor with methods (getValue, setValue) that return Observables.

Finally, I'm really excited to see things starting to take shape for Firebase and Angular 2. Looking forward to the future of this project :sunglasses:

jeffbcross commented 8 years ago

What's the advantage of using a @FirebaseList or @FirebaseObject decorator over a service e.g. new Firebase( pathAndOptions ) or getFirebase( pathAndOptions )?

@TylerEich good question. To answer the "why a decorator" part of the question, using a decorator makes code more statically-analyzable for tooling to be built in the future (tooling for refactoring, stubbing out components with Firebase integration, etc).

I noticed that Falcor makes extensive use of Observables, but not by subclassing. They use a Model constructor with methods (getValue, setValue) that return Observables.

I wanted a succinct, flat-as-possible API. And @blesh, maintainer of RxJS 5, has officially told me that he approves of subclassing Observable in this way.

Finally, I'm really excited to see things starting to take shape for Firebase and Angular 2. Looking forward to the future of this project :sunglasses:

Me, too!

benlesh commented 8 years ago

I wanted a succinct, flat-as-possible API. And @blesh, maintainer of RxJS 5, has officially told me that he approves of subclassing Observable in this way.

Just remember that 50% of the reason Observable is a class is so you can put operators on it. The meat of it is really just a function. The other 50% of the reason is so you can derive Subject from Observable.

What I wouldn't do: Subclass Observable and start building a highly stateful type out of it with a bunch of methods that affect that state. If you're doing that, you might want to subclass Subject, depending on what your goals are.

For example:

interface FireNotification<T> {
  type: string,
  data: T
}

class FireSubject<T> extends Subject<FireNotification<T>> { 
  next: (notification: FireNotification<T>) => void;
  error: (err: any) => void;
  complete: () => void;

  subscribe(observerOrNext?: (notification: FireNotification<T>) => void | Observer<FireNotification<T>>,
    error?: (err: any) => void,
    complete?: () => void): Subscription;
}

This would enable you to send like { type: 'delete', data: someRecord } or recieve { type: 'update', data: { whatever: 'here' } }. And do so in a composable way.

jeffbcross commented 8 years ago

Achievement unlocked: a comment from @blesh .

What I wouldn't do: Subclass Observable and start building a highly stateful type out of it with a bunch of methods that affect that state.

In this case, the mutation methods are just proxying mutations to the Firebase ref, which persists the change and then emits an event such as child_added once it has satisfactorily persisted the change. So the Observable itself isn't tracking state, other than an array that is internally kept in sync as child_moved, child_added, and child_removed events are emitted from Firebase.

jeffbcross commented 8 years ago

Update: I've updated angular/angular issue #6643 to describe how @tbosch recommends we approach this with Dependency Injection instead of a decorator, giving us the same benefits without hacks/side-effects. There's one catch: DI can't do what I need right now, and it may not be easy to convince others on the team that it should.

robwormald commented 8 years ago

@blesh's idea is roughly how I've implemented redux-in-rx-for-ng2 and it works pretty well so far.

in effect, its basically something like:


actions$.scan(firebaseReducer, []).subscribe(arr => { ... });

where actions$ is a stream of events like { type: 'update', payload: data }

and firebaseReducer is a function that looks something like below and is responsible for keeping the internal array in sync with the remote


const firebaseReducer = (list = [], action) => {
  switch(action.type){
    case 'update':
      return list.map(item => item.id === action.payload.id ? Object.assign({}, item, action.payload) : item);
    case 'delete':
      return list.filter(item => item.id !== action.payload.id);
    //etc
    default:
      return list;
  }
}

This is wrapped up into a Store (which is roughly what I think a FirebaseRef would become):

class FirebaseRef {
  constructor(url: string);
  next(action): void;
}

which I reckon for ease of use might be extended as (where push() just proxies to next() and appropriately wraps in an Action)

class FirebaseRef {
  constructor(url: string);
  next(action): void;
  push(data):void;
  delete(data):void;
  //etc
}
benlesh commented 8 years ago

@robwormald this is off-topic, but sort of feel like switch statements in reducers are an anti-pattern. I'd rather see a stream of actions be scanned into state. I'm sure this has been thought of by @gaearon, but just to put in out there:

some pseudocode:

// assume all Action types below implement this:
interface Action<S> {
  act (state: S): S
}

const updates = updateButtonClicks.map((id) => new UpdateAction(id, getUpdatesFromForm()));
const deletes = deleteButtonClicks.map((id) => new DeleteAction(id));

const actions = Observable.merge(updates, deletes);

const states = actions.scan((state, action) => action.act(state), initialState);

(obviously the "classness" of UpdateAction and DeleteAction are totally optional)

benlesh commented 8 years ago

... after that, it becomes a matter of adding Action types by convention, rather than updating a switch statement, which is more "configuration" than "convention".

gaearon commented 8 years ago

We use plain object actions in Redux so they are easy to record, serialize, and replay. Classes kill that because nobody will bother to implement deserialize() or something like this. More on Redux “boilerplate” design decisions and how they are driven by use cases we care about: http://redux.js.org/docs/recipes/ReducingBoilerplate.html

Actions are plain objects describing what happened in the app, and serve as the sole way to describe an intention to mutate the data. It’s important that actions being objects you have to dispatch is not boilerplate, but one of the fundamental design choices of Redux.

There are frameworks claiming to be similar to Flux, but without a concept of action objects. In terms of being predictable, this is a step backwards from Flux or Redux. If there are no serializable plain object actions, it is impossible to record and replay user sessions, or to implement hot reloading with time travel. If you’d rather modify data directly, you don’t need Redux.

benlesh commented 8 years ago

:) I wasn't at all concerned with boilerplate and I'm not married to classes, either.

Replay-ability is an interesting concern, and I see what you mean. Seems like there should be a solution that caters both to replay an maintainability, as a switch statement could get out of hand and large very quickly on some large apps. Perhaps the plain object could be used to look up particular reducer function by convention?

benlesh commented 8 years ago

Either way, I'm horribly off-topic for this thread. :)

benlesh commented 8 years ago

I'll take this off-line.

gaearon commented 8 years ago

Sorry for hijacking the thread, this is my last comment here :-)

as a switch statement could get out of hand and large very quickly on some large apps

This is not true in my experience. You're not actually supposed to write a single reducer function that manages your whole app. You split it into many reducer functions that manage parts of the app's state. Effectively most reducers manage about 5 different actions, and they are composed into other reducers, and so on, up to the root reducer. A giant switch is an anti-pattern, just like a giant component. You're supposed to split them.

benlesh commented 8 years ago

(meta: @gaearon showed me what he means and he's right) (but I still like a more functional approach than switch statements I think) (maybe) (I dunno)

jeffbcross commented 8 years ago

Thx for putting @blesh in his place, @gaearon. I've tried and failed many times.

benlesh commented 8 years ago

TIL: I have a place.

katowulf commented 8 years ago
@FirebaseList({
  path: 'https://<FIREBASE>/posts',
  query: [['orderByChild', 'timestamp'], ['limitToFirst', 2]]
})

Note that this removes a lot of the flexibility of the current AngularFire design, which allows it to work with complimentary tools like Scroll, NormalizedCollection, MockFirebase, and others. The purpose of these libs is unclear in the future of Firebase, but the point is that such things will exist, and they plug instantly into AngularFire's current design.

It's probably fine to extend FirebaseList in some manner to add in these items, but worth keeping in mind the difficulty incurred vs the benefits of passing a string and query parameters instead of just a Firebase ref.

One idea to ponder would be allowing both. E.g. pass either String path or Firebase ref.

jeffbcross commented 8 years ago

Closing in favor of #39