szaranger / firebase-saga

A library for connecting redux-saga middleware to Firebase.
58 stars 15 forks source link

events in sync #8

Closed brandonmp closed 8 years ago

brandonmp commented 8 years ago

two questions on this code:

function* runSync(ref, eventType, actionCreator) {
    const opts = newOpts();
    yield call([ref, ref.on], eventType, opts.handler);

    while (true) {
        const { data } = yield take(opts);
        yield put(actionCreator({ key: data.key, value: data.val() }));
    }
}

/**
 * Gets fired every time a child added, remove, changed, or moved
 *
 * @param path
 * @param mapEventToAction
 * @param limit
 * @example
 * import { sync, CHILD_ADDED, CHILD_REMOVED } from 'firebase-saga';
 *
 *function* syncPosts() {
 *   yield fork(sync, 'posts', {
 *       [CHILD_ADDED]: actions.syncPostAdded,
 *       [CHILD_REMOVED]: actions.syncPostRemoved
 *   });
 *}
 */
export function* sync(path, mapEventToAction = {}, limit = 20) {
    const ref = firebase.database().ref(path).limitToLast(limit);

    for (let type of EVENT_TYPES) {
        const action = mapEventToAction[type];

        if (typeof action === 'function') {
            yield fork(runSync, ref, type, action);
        }
    }
}
  1. why the default value on limit? i would assume the default use case would be no limit. would this be more general?
export function* sync(path, mapEventToAction = {}, limit = 20) {
    const ref = typeof limit === 'number' ? 
           firebase.database().ref(path).limitToLast(limit)
           : firebase.database().ref(path)
// ...
  1. the readme says sync covers all events, but const EVENT_TYPES = ['child_added', 'child_removed'];

is that a bug or is it meant to only cover add/remove?

i can submit prs for this stuff, just let me know !

brandonmp commented 8 years ago

actually maybe something like this would let users pick their own events by declaring an action creator for it. what do you think? I also added value event, was ther a reason for not including that?

const EVENT_TYPES = ['child_added', 'child_removed', 'value', 'child_changed', 'child_moved'];
export function* sync(path, mapEventToAction = {}, limit = 20) {
    const ref = typeof limit === 'number' ? 
           firebase.database().ref(path).limitToLast(limit)
           : firebase.database().ref(path)

    let eventTypes = Object.keys(mapEventToAction)
    for (let type of eventTypes) {
        // make sure event types are valid
                if (EVENT_TYPES.indexOf(type) === -1) break;

        const action = mapEventToAction[type];

          if (typeof action === 'function') {
            yield fork(runSync, ref, type, action);
        }
    }
}
szaranger commented 8 years ago

Hey @brandonmp,

Thanks for your suggestions, and I have made the following changes in response:

  1. Removed default limit in sync
  2. Added value to the EVNET_TYPES
  3. Sync now covers all events
  4. Had to remove transform-runtime as it was giving issues when installing packages. This needs to be revisited