djhi / my-nutrition

A Meteor application using Webpack, React and Redux for nutritionists who coach people.
MIT License
79 stars 25 forks source link

Redux-meteor discussion #3

Open jptissot opened 8 years ago

jptissot commented 8 years ago

Hey, dunno if you looked at this already. Looks interesting https://github.com/zhongqf/meteoredux

djhi commented 8 years ago

Yes, I remember this project. I think what it does inside a reducer should go in a middleware. Reducers should not be aware of Meteor collections.

kevohagan commented 8 years ago

I totally agree with @djhi , ive just been reading through https://medium.com/@meagle/understanding-87566abcfb7a

also as you said before @djhi the middlewares folder could/should definitely be a npm package.

im barely getting started with redux, but this stack seems very promising. obviously the fork of @jedwards1211 repo for webpack integration still seems a bit tricky/hacky and hopefully this will change in the future with new versions of meteor (npm integration, webpack, es6 modules, etc ) .

btw you should anounce this project on meteor forums to get some visibility and feedback :)

great job @djhi ! :)

djhi commented 8 years ago

Thanks :)

I'll make npm packages for the middlewares when I'll be convinced there are no better ways to do them. I'm still trying to figure out what @jedwards meant by making them hot reload compatibles.

I totally agree about it being a "little" hacky but there are no other ways to do this in order to get "standard" npm and webpack integration right now. I'd like meteor to just be a collection of npm packages, maybe with suffixes like meteor-accounts-server, meteor-account-client, referencing meteor-account-common if things must be shared on both sides, and a meteor binary to ease buildings, deploying, etc.. That would be great.

I'm not sure I want that projet to be that visible so soon because I'm not really happy about the React part. But I do need feedbacks as it may lead to better ways to do it. Maybe I'll do in one week.

kevohagan commented 8 years ago

sure :)

btw just fyi , i find your setup and proposed pull-request cleaner than jedwards one :)

On Sat, Nov 28, 2015 at 2:37 PM Gildas Garcia notifications@github.com wrote:

Thanks :)

I'll make npm packages for the middlewares when I'll be convinced there are no better ways to do them. I'm still trying to figure out what @jedwards https://github.com/jedwards meant by making them hot reload compatibles.

I totally agree about it being a "little" hacky but there are no other ways to do this in order to get "standard" npm and webpack integration right now. I'd like meteor to just be a collection of npm packages, maybe with suffixes like meteor-accounts-server, meteor-account-client, referencing meteor-account-common if things must be shared on both sides, and a meteor binary to ease buildings, deploying, etc.. That would be great.

I'm not sure I want that projet to be that visible so soon because I'm not really happy about the React part. But I do need feedbacks as it may lead to better ways to do it. Maybe I'll do in one week.

— Reply to this email directly or view it on GitHub https://github.com/djhi/my-nutrition/issues/3#issuecomment-160296395.

djhi commented 8 years ago

Thanks, I'm just applying some patterns I learned since I work at @marmelab. jedwards did all the hard work !

jptissot commented 8 years ago

After watching the egghead.io redux videos and browsing through your code, I now understand why it doesn't make sense to have Meteor collections in reducers. I like the approach you took in the middleware. I wonder if there is a way to use constants for the different actions that are dispatched from the middlewares (such as the ${action.type}_CHANGED action type) instead of relying on string constants ?

djhi commented 8 years ago

Yes, there is a way. I borrowed this from @robinbressan. You define an helper object with methods like this to build action types:

const typeBuilder = {
    type: actionType => return `@myprefix/${actionType}`, // prefix is used to avoid potential conflicts with other libs
    ready: actionType => return `${actionType}/ready`,
    changed: actionType => return `${actionType}/changed`,
}

We could also define typeBuilder as a function taking the prefix as a parameter of course.

You then declare your actions types like this:

export const MyActionType = typeBuilder.type('MyActionType');

export function MyAction = parameter => ({
    type: MyActionType,
    parameter,
})

And the middleware could use the typebuilder too when dispatching ready and changed:

dispatch(typeBuilder.ready(action.type);

dispatch(typeBuilder.changed(action.type);

Same for reducers. Neat, isn't it ?

djhi commented 8 years ago

I'll introduce this very soon in the project. Just need to find time for it...

genyded commented 8 years ago

There's another nice Meteor, Webpack, React starter here that even supports SSR but it has many of the same challenges and I had a hard time trying to integrate redux into it because they actually hide the <Router> elemenent inside their SSR renderer.

genyded commented 8 years ago

Just an update... I've been working with the devs of the reactivestack Meteor, Redux, Webpack starter and it now has full Redux support. I have also fully integrated it with this repos middelware, actiontypebuilder and other cool stuff and it makes for a very solid foundation.

The only current drawback is that the reactivestack does not have a standard package.json file, but that will change when Meteor 1.3 comes out. Otherwise it fully supports code splitting, standard Meteor integration (without all the complex scripting of the jedwards stuff - to run it you just type meteor as with any meteor app) and when integrated with the core features of this repo (middleware, action builders, etc), is very cool to work with.

dbismut commented 8 years ago

Hey @genyded, I'm just curious: are you using a middleware to dispatch collection changes similarly to what @djhi is doing in MeteorSubscription.js? If so, how do you stop subscriptions when the component is unmounted? This seems to be missing from the current implementation...?

genyded commented 8 years ago

Hi David, I assume you mean the Meteor subscriptions? If so, subscriptions have a stop() method that I invoke via a seperate action in the components unmount. The middleware already has handles for each subscription so you just need to modify it to stop when the additional action is invoked. You have to change a few things to get it all working, but it's almost the opposite of subscribe (without ready and changed) from workflow perspective.

genyded commented 8 years ago

@dbismut If it helps... here is what I changed (or added)....

@djhi meteorSubscription.js...

...
export default store => next => action => {
  if (!action.meteor || !action.meteor.subscribe && !action.meteor.unsubscribe) {
    return next(action);
  }
  console.log('ACTION: ', action)
  const { subscribe, get, onChange, unsubscribe } = action.meteor;

  // If we already have an handle for this action
  if (handles[action.type]) {
    const subscriptionId = handles[action.type].subscriptionId;
    computations[subscriptionId].stop();
    handles[action.type].stop();
    //if unsubscribing, dispatch needed actions and exit 
    if(unsubscribe) {
      store.dispatch({
        type: actionTypeBuilder.ready(action.type),
        ready: false,
      });
      store.dispatch({
        type: actionTypeBuilder.unsubscribe(action.type),
        data: [],
      });
      return;
    }
  }

  const handle = subscribe();
  const subscriptionId = handle.subscriptionId;
  handles[action.type] = handle;

  computations[subscriptionId] = Tracker.autorun(() => {
    const data = get();
    const ready = handle.ready();

    store.dispatch({
      type: actionTypeBuilder.ready(action.type),
      ready,
    });

    if (ready) {
      if (onChange) {
        onChange(data);
      }

      store.dispatch({
        type: actionTypeBuilder.changed(action.type),
        data,
      });
    }
  });
};

@djhi actionTypeBuilder.js...

export function actionTypeBuilder(prefix) {
  return {
    type: actionType => `${prefix}/${actionType}`,
    ready: actionType => `${actionType}/ready`,
    changed: actionType => `${actionType}/changed`,
    error: actionType => `${actionType}/error`,
    unsubscribe: actionType => `${actionType}/unsubscribe`,
  };
}

export default actionTypeBuilder('@my-nutrition');

As I mentioned, I then dispatch an unsubscribe in the unmount...

componentWillUnmount() {
    const { actions } = this.props;
    actions.someSubscriptionUnsubscribe();
  }

That action creator looks something like this and MUST be passed the same type as you used to subscribe...

export function someSubscriptionUnsubscribe() {
    return dispatch => {
      dispatch({
        type: MY_SUBSCRIPTION, //<- THIS MUST BE THE SAME TYPE PASSED IN ON THE SUBSCRIBE
        meteor: {
          //passing in a dummy string here so this is not null/empty
          unsubscribe: 'unsubscribe'
        }
      });
    };
}

You should see everything logged for the actions if you are monitoring them and if you want to clean up the state you can do something like...

...
case actionTypeBuilder.unsubscribe(MY_SUBSCRIPTION):
      return {
        ...state,
        items: data //this will set items to an empty array
      };
...
dbismut commented 8 years ago

@genyded thanks, it does help a lot. I was hoping there would be some way to hijack the connect function to behave like composeWithTracker.

A few things on my side: maybe you should let the unsubscribe action decide what data the middleware should pass back to state.items (in my case I sometimes use findOne instead of fetch and expect the data to be an object and not an array). So you would probably use:

export function someSubscriptionUnsubscribe() {
  return dispatch => {
    dispatch({
      type: MY_SUBSCRIPTION, 
      meteor: {
        unsubscribe: 'unsubscribe',
        items: initialState.items
      }
    });
  };
}

Also, don't you feel it's weird to keep track of the old computation in computations[subscriptionId].stop()? Shouldn't it be deleted?

And one last thing: when I logout from my app, I'm automatically redirected to a signup page. When I signup again (without reloading the page), I'm redirected to the page I was logged to, but the subscriptions don't occur, and my data is not loaded.

My way of solving this is to check if there is already a similar subscription and skip the new subscription (instead of stopping the existing one and generating another one).

genyded commented 8 years ago

@dbismut It's probably possible to do something like composeWithTracker, but right now I do not have time to boil the ocean and was trying to get something working quickly.

1) You write the reducers, so you can in fact set set items (or whatever state you care about) to anything you want. However, it seems like the middleware should still set data to some reasonable default instead of leaving it as some potentially large object or array. Maybe undefined is a better option tough. Also note that setting data during unsubscribe has no direct effect on state unless you actually pass it in the reducer.

2) Meteor computations are not really well documented (nor is 'cleanup' for handles) other than a portion that states:

Each call to Tracker.autorun creates a new Computation object, which represents the autorun. Calling the stop() method on the Computation 'cleans up' the autorun and stops it from ever rerunning again.

... but I supposed you could do something like setting them both to undefined in the unsubscribe portion of the middleware:

if(unsubscribe) {
      computations[subscriptionId] = undefined;
      handles[action.type] = undefined;
      store.dispatch({
        type: actionTypeBuilder.ready(action.type),
        ready: false
      });
      store.dispatch({
        type: actionTypeBuilder.unsubscribe(action.type),
        data: [],
      });
      return;
   }

That shouldn't have any side effects that I can think of, but there are no delete() or other cleanup methods for either computations or handles - only stop(). That seems like a Meteor issue to solve though. They may be garbage collected at some point, but are both still populated immediately after calling stop() on both (you can see this by logging them somewhere).

3) Regarding the logout and login, not entirely clear on the point of how that relates to what I posted. Again, you control what happens in those scenarios, not any repo.

dbismut commented 8 years ago

@genyded Of course, I'm not expecting for you to come up with solutions in what I'm trying to achieve, from what I can see we stumble upon more or less the same threads - I guess we're trying to integrate the same things (i.e. meteor, webpack, redux) so that's why I was eager to have that discussion with you.

  1. Yes, I agree, there should be a default.
  2. I think computation[subscriptionId].stop(); delete computation[subscriptionId] should work since we're not ever using computation[subscriptionId] afterwards.
  3. My point is connected to point number 2 actually. I'll try to make it clearer:
    • when I log out from the app, all data from the subscriptions is cleared
    • when I log back again, subscriptions are supposed to be stopped and renewed, and data should be reloaded. It looks to me like when the subscriptions are renewed, the subscription handles never get to the ready state (and the ready action is never dispatched).

And the way I solved the issue is with the following code (not implementing unsubscribe yet):

export default store => next => action => {
  if (!action.meteor || !action.meteor.subscribe) {
    return next(action);
  }

  const { subscribe, get, onChange } = action.meteor;

  // if a subscription already exists for that action type, don't do anything
  // there is no stop() for existing subscriptions

  if (!handles[action.type]) {  
    const handle = subscribe();
    const subscriptionId = handle.subscriptionId;
    handles[action.type] = handle;

    computations[subscriptionId] = Tracker.autorun(() => {
      const ready = handle.ready();

      if (ready) {
        const data = get();
        if (onChange) {
          onChange(data);
        }
        store.dispatch({
          type: actionTypeBuilder.changed(action.type),
          data,
          loaded: !_.isEmpty(data)
        });
      }
      store.dispatch({
        type: actionTypeBuilder.ready(action.type),
        ready
      });
    });
  }
};

I think this is a bad idea since it prevents from changing subscription arguments, but for now it solves my logout issue. I'll try to dig further though to understand what's happening, but I was wondering if the same would happen in your situation (again, not knowing what your implementation looks like).

Is that any clearer?

Cheers.

genyded commented 8 years ago

@dbismut Cool - always happy to chat about whatever! We also do seem to be moving somewhat in sync. I agree using delete in favor of setting them to unassigned is better for handles and computations.

Thanks for clarifying number 3! Not sure what may be going on there yet either, but I get data (ready->change) on subsequent reloads whether or not I unsubscribe first.

Based on your last code sample though, it looks to me like if (!handles[action.type]) would only get run the first time? If you are not stopping and clearing the initial handle then on a second or third or ??? run, wouldn't handles[action.type] still be set this bypassing the entire code block?

dbismut commented 8 years ago

Yes, handles[action.type] would only run the first time (until you explicitly unsubscribe and also delete handles[action.type] to allow for new subscriptions). Typically, if you subscribe to Meteor.subscribe('posts'), and then resubscribe to Meteor.subscribe('posts') later without unsubscribing first there is no reason why you would stop the subscription and re-run a new one I guess!

It's far from ideal, since that would prevent new subscriptions from the same action type but with different arguments (like Meteor.subscribe('posts', postNumberOne) and Meteor.subscribe('posts', postNumberTwo) to re-run.

But again, the issue only seems to happen when logging out with Meteor.logout() and signing back in... Anyway! Will try to understand what's wrong and let you know my findings if this is of any interest!

genyded commented 8 years ago

I (and maybe others) would like to know what you find if you have time. May help us if we run into it in the future!

dbismut commented 8 years ago

Hey - so here are my findings. @genyded, I guess the reason why you don't get my issue is because you dispatch the unsubscribe action when your component unmounts and that stops the subscription and computation.

Therefore, these bits of code never actually run in sequence:

  computations[subscriptionId].stop();
  handles[action.type].stop();
    // <-- here is the unsubscribe action that stops the code right there
  const handle = subscribe();
  const subscriptionId = handle.subscriptionId;
  handles[action.type] = handle;
  computations[subscriptionId] = Tracker.autorun(() => { ...});

But if you dispatch the same subscribe action twice (without unsubscribing), then you should see the same bug I see, in other words that the new subscription will never get ready.

EDIT: I think I'm close to a working solution, I'll post this tomorrow.

dbismut commented 8 years ago

Ok so here it is. It's not extensively tested, but it seems to work. Essentially the idea is to make it work similarly to how Meteor handles subscriptions internally, with the use of params which is the parameters given to the subscribe function.

The actions to be dispatched should be structured as follow:

// action.js
export function loadItems(postCollection) {
  return (postId) => {
    return dispatch => {
      dispatch({
        type: ITEMS,
        meteor: {
          subscribe: true, // tells the middleware that this is a subscribe action
          name: 'posts', // name of the subscription
          params: [postId], // parameters to be passed to the subscribe method
          get: () => postCollection(postId).fetch(),
          initialData: [] // this is to indicate what the data should be reverted to when the subscription stops
        }
      });
    };
  };
}

And here is the middleware.

// middlewares/meteorSubscription.js
import actionTypeBuilder from '../actions/actionTypeBuilder';

/*
    subs will keep track of existing subscriptions and will be structured as follows:

    subs[action.type] = {
      name, // name of the subscription we subscribe to
      params, // parameters of the subscription (like in Meteor.subscribe(name, [params]))
      handle, reference to the subscription handle,
      computation reference to the autorun computation for the query get
    };
*/

const subs = {};

export default store => next => action => {
  if (!action.meteor || (!action.meteor.subscribe && !action.meteor.unsubscribe)) {
    return next(action);
  }

  const { name, get, initialData, unsubscribe, onChange } = action.meteor;
  const params = action.meteor.params || [];

  const sub = subs[action.type];

  if (sub && unsubscribe) {

    // If there is an existing subscription and the unsubscribe action
    // has been dispatched, we stop the subscription.

    // The onStop callback will take care of dispatching the unsubscribe action.

    sub.handle.stop();
    return;
  }

  // We check if there is an existing subscription with the same subscription name and 
  // same params that is running with the same action.type

  const existing = sub && sub.name === name && _.isEqual(sub.params, params);

  if (! existing) {
    // Here we know that a similar subscription (same collection, same params)
    // hasn't been already dispatched

    // If `existing` wasn't undefined, so if there was an existing subscription with
    // the same subscription name and same params as the action dispatched
    // then there would be no reason to resubscribe
    // to the same subscription with same params!

    if (sub) {
      // However, if there is a subscription that was dispatched
      // with the same action.type, we stop it

      sub.handle.stop();
    }

    // Let's take care of the new subscription

    const handle = Meteor.subscribe(name, ...params, {
      onStop: () => {

        // When the subscription stops for whatever reason,
        // we stop the autorun computation and we dispatch the unsubscribe action

        // This is cool since the unsubscribe action is also dispatched
        // if the subscription stops for any reason

        subs[action.type].computation.stop();
        store.dispatch({
          type: actionTypeBuilder.unsubscribe(action.type),
          data: initialData || undefined
        });
        store.dispatch({
          type: actionTypeBuilder.ready(action.type),
          ready: false
        });
        delete subs[action.type];
    }});

    subs[action.type] = {
      name,
      params: _.clone(params),
      handle: handle
    };

    // the rest is similar 
    subs[action.type].computation = Tracker.autorun(() => {
      const ready = handle.ready();

      store.dispatch({
        type: actionTypeBuilder.ready(action.type),
        ready
      });

      if (ready) {
        const data = get();
        if (onChange) {
          onChange(data);
        }
        store.dispatch({
          type: actionTypeBuilder.changed(action.type),
          data
        });
      }
    });
  }
};
genyded commented 8 years ago

Cool stuff! I put it through some wringers and everything I tried worked just fine. Nice job! The only thing I noted is that you are not importing lodash in the modified middleware. So, I assume you have a meteor global lodash (or underscore) which is fine. However that will not work 'out of the box' with this repo because it only has an NPM lodash depedency, so it must be imported to use it or or meteor global of one or the other must be added.

Otherwise this seems like a good candidate for a PR and/or NPM package of it's own (probably not worth Meteor packaging/wrapping it because 1.3+ will use standard modules)! Thanks for taking the time to do and share this!

djhi commented 8 years ago

Glad to see you guys thinking about improving this meteor/redux integration! I'll take time to read the full discussion this wk. Have you read the How we redux articles by Abhi Aiyer yet ? I've tried to applied his workflow to a small project but using meteor 1.3 and that was way more simple. I'd just like to get rid of the meteor mixin. More on that later :)

dbismut commented 8 years ago

@genyded You're right, I'm using underscore which is exposed globally by Meteor. Note that I'm using the kickstart-hugeapp bootstrap and not jedwards' as @djhi (you're doing the same thing as well right?). And since it allows for server-side rendering, there are some tweaks I needed to make to make sure it would work in production. I can share if you're interested, but it's not exactly related to this repo :)

@djhi well yes, I had read them quickly. Does the simplicity improvement in your small project workflow comes from being able to use Meteor 1.3 or from Aiyer's redux implementation? I guess most of the complexity in your repo comes from removing the meteor mixin and replacing it with a middleware, which is what you did and what I genuinely like and precisely what he doesn't (see this discussion :). Also the fact that you removed all global declarations for collections clearly adds a layer of complexity (but again, I also like it).

I think a cool improvement would be to inject subscriptions and unsubscriptions directly in react-redux connect wrapper element within the componentWillMount and componentWillUnmount methods. That would allow writing pure components that would use the subscription data in their props.

dbismut commented 8 years ago

Here is the new implementation I'm trying, which I think makes more sense.

I'm not sure it works, I haven't tested it extensively, but I'm removing the ready action. I don't think it's really necessary to dispatch a ready action, and then a changed action. The changed reducer should take the responsibility of setting the state to ready if it wants to. Similarly, the unsubscribe reducer should be responsible for setting the ready state to false, and the data to whatever it wants.

I'm using a Promise as I want to try to better handle server side rendering with fetchData, as indicated here: https://github.com/rackt/redux/issues/99

import actionTypeBuilder from '../actions/actionTypeBuilder';

const subs = {};

export default store => next => action => {
  if (!action.meteor || !action.meteor.subscribe) {
    return next(action);
  }

  const { name, get, params, unsubscribe, onChange } = action.meteor.subscribe;
  const parameters = params || [];

  return new Promise((resolve, reject) => {

    if (Meteor.isServer) {
        Meteor.subscribe(name, ...parameters);
        next( { type: actionTypeBuilder.changed(action.type), data: get() });
        return resolve();
    }

    const sub = subs[action.type];

    if (sub && unsubscribe) {
      sub.handle.stop();
       if (subs[action.type] && subs[action.type].computation) {
         subs[action.type].computation.stop();  
       }
      delete subs[action.type];
      next({ type: actionTypeBuilder.unsubscribe(action.type) });
      return resolve();
    }

    const existing = sub && sub.name === name && _.isEqual(sub.parameters, parameters);

    if (existing && sub.handle.ready()) {
      return resolve();
    }

    if (sub && sub.handle.ready()) {
      sub.handle.stop();
    }

    next({ type: actionTypeBuilder.loading(action.type) });

    const handle = Meteor.subscribe(name, ...parameters, {
      onReady: () => {
        subs[action.type].computation = Tracker.autorun(() => {
          const data = get();
          if (onChange) {
            onChange(data);
          }
          next({ type: actionTypeBuilder.changed(action.type), data: data  });
          return resolve();
        });
      },
      onStop: (error) => {
        if (error) {
          next({ type: actionTypeBuilder.error(action.type), error });
          return reject(error);
        }
        if (subs[action.type] && subs[action.type].computation) {
          subs[action.type].computation.stop();  
        }
      }
    });

    subs[action.type] = {
      name,
      parameters: _.clone(parameters),
      handle: handle
    };
  });
};

EDIT: some tweaks - contrary to what I've said above, you wouldn't want to get the unsubscribe action whenever the subscription stops. For example, if you change arguments for the subscription, the previous subscription handle will stop and a new one will be created. In the previous solution, an unsubscribe action would therefore have been dispatched, probably resetting the data of the state (depending on your reducer), then a changed action would set the state data to the get() query result. What we actually want is only the state data to change to the new subscription result, and not to be reset in the process!

genyded commented 8 years ago

@dbismut - Hey David, I'll try to find some time to play with this over the weekend. I am curious if you'd recommend a particular Promise library/package? Also is there a particular reason you went the Promise route instead of something like Meteor.wrapAsync()?

Thanks!

dbismut commented 8 years ago

@genyded unfortunately I'm really genuinely new to all this, so I'm really afraid I'll give you a wrong explanation.

But it seems to me that Promises are part of es6 so no need for a package AFAIK.

However, I did need to add meteor add promises so that react-router-ssr can use Promise.awaitAll on the server.

Not sure about meteor.wrapAsync. I need to return a promise, not sure wrapAsync can do this...

genyded commented 8 years ago

Thanks again! Got it. Just FYI - 'wrapAsync' is sort of a Meteor-ish way to do promise like calls (make Async appear as sync). IMHO it's not as 'clean' or straight forwards as Promises (and often requires bringing Fibers into the mix), but it is Meteor's 'way'.

Recent occurrences in the JS community as a whole sometime have me wondering (and probably MDG too) what parts of an app should be Meteor and what should be 'something else'. We saw evidence of this with React (versus Blaze) and I think this is also something that falls under that umbrella (promises versus wrapAsync).

Not saying anything or either is right or wrong, just that this too has a couple of ways of approaching it. I personally tend to favor Promises as well, but others may not because it's yet another non-Meteor API to learn and does add more code. Just don't want others who may read all this to think anyone is saying this is the only (or maybe even the 'right') way to handle things.

I am anxious to see what unfolds as the ultimate preferred 'mold' for a Meteor, React, Webpack, Babel, Redux, GraphQL, Relay, Flow, ??? app. LOL. It'll probably be good/valid for about a month ;-)

dbismut commented 8 years ago

True I agree that this is probably a lot of headaches just for something that is going to be outdated in a few weeks ^^ Who knows what MDG will come up with React integration in Meteor 1.3+.

'wrapAsync' is sort of a Meteor-ish way to do promise like calls (make Async appear as sync). IMHO it's not as 'clean' or straight forwards as Promises (and often requires bringing Fibers into the mix), but it is Meteor's 'way'.

wrapAsync only uses futures on the server (and I guess is mainly useful on the server) to make async functions from npm packages behave in sync.

Recent occurrences in the JS community as a whole sometime have me wondering (and probably MDG too) what parts of an app should be Meteor and what should be 'something else'.

Me too... At the moment, I'm trying to make my app as agnostic from Meteor as it can be, using Meteor functions only in middlewares. At the expense of duplicating the data state from minimongo in redux as in the nutrition app from @djhi

Also by the way, some actions in the app use thunks when I'm not sure it's necessary. For example at L11 in meals.js, this can be rewritten:

export function deleteMealFactory(collection) {
  return id => {
    return dispatch => { // <--- this dispatch is not necessary
      dispatch({
        type: MEALS_REMOVE,
        meteor: {
          remove: {
            id,
            collection,
          },
        },
      });
    };
  };
}

I think this could be rewritten:

export function deleteMealFactory(collection) {
  return id => {
    return {
      type: MEALS_REMOVE,
      meteor: {
        remove: {
          id,
          collection,
        },
      },
    }
  }
}
dbismut commented 8 years ago

And here is the new meteorMethod.js:

// meteorMethod.js
import actionTypeBuilder from '../actions/actionTypeBuilder';

export default store => next => action => {
  if (!action.meteor || !action.meteor.call) {
    return next(action);
  }

  const { method, params } = action.meteor.call;
  const parameters = params || [];

  const meteorMethod = typeof method === 'string' ? Meteor.call : method;

  if (typeof method === 'string') {
    parameters.unshift(method);
  }

  return new Promise((resolve, reject) => {

    next({ type: actionTypeBuilder.loading(action.type) });

    meteorMethod(...parameters, (error, result) => {
      if (error) {
        next({ type: actionTypeBuilder.error(action.type), error });
        return reject(error);
      }

      next({ type: actionTypeBuilder.success(action.type) });
      return resolve(result);
    });
  });
};

That allows writing actions such as:

// auth.js
export function signIn(email, password) {
  return {
    type: USER_SIGNIN,
    meteor: {
      call: {
        method: Meteor.loginWithPassword,
        params: [email, password]
      }
    }
  }
}

// example chaining actions
export function getIpAddress() {
  return {
    type: USER_GETIPADDRESS,
    meteor: {
      call: { method: 'users.getIPAddress' }
    }
  }
}

export function signUp(email, username, password, captcha) {
  return {
    type: USER_SIGNUP,
    meteor: {
      call: {
        method: Accounts.createUser,
        params: [{ email, username, password, captcha}]
      }
    }
  }
}

export function signUpWithCapcha(email, username, password, captchaValue) {
  return dispatch => {
    return dispatch(getIpAddress()).then((remoteip) =>
      dispatch(signUp(email, username, password, { value: captchaValue, remoteip }))
    );
  }
}

So far so good, and everything related to redux now looks similar to what redux guys are doing :)

djhi commented 8 years ago

@dbismut You're right about the unnecessary dispatch.

You did a great job reviewing the middlewares ! Can you submit a PR ? Thx !

dbismut commented 8 years ago

Hey @djhi, unfortunately I'm not familiar with jedwards1211/meteor-webpack-react, I'm using kickstart-hugeapp. Your project has been immensely useful in helping me understanding redux, but I think it's too complex for me to be able to submit a PR without breaking it.

I'm thinking of publishing a repo with my own implementation of meteor / react / redux (largely based on what you did), but I'm really scared I would mess your app with the new middlewares and associated actions and reducers.

dbismut commented 8 years ago

Ok. My latest attempt, for reference if it helps anyone :) https://gist.github.com/dbismut/35b89185998d68bc18fd

Again, not tested extensively, but should show the general direction!!

Main changes: there is now a subscriptions state attribute that handles the state of subscriptions via a middleware. That middleware then delegates the autorun query to the datasource middleware.

djhi commented 8 years ago

thx for sharing :)

genyded commented 8 years ago

@dbismut - Hey David,

Finally got a chance to try out your latest and overall looks very cool! I am curious (and unless I missed something did not see an example) how you are handling 'unsubscribe'?

dbismut commented 8 years ago

Hey @genyded, cool that you like it! To be honest, I haven't really tested unsubscribe just yet, but essentially it should work just by dispatching an action like this:

export function unsubscribe(type, name, ...params) {
  return {
    type: type,
    meteor: {
      unsubscribe: { name: name }
    }
  };
}

...where type should be the same type as the type of the subscribe action you've dispatched. I would really love to automatize unsubscribing on componentWillUnmount but I don't know how just yet.

What I really really like about the way everything works now is the fact that the actions (whether they are related to subscriptions or Meteor.call are now Promises. This is so easy to work with packages such as redux-form, and even with react-router fetchData for SSR. When I finish what I'm doing now, I'm planning to release a bootstrap project where I hope everything will be clear. I'm pretty sure it can be very much improved :)

genyded commented 8 years ago

Cool - I figured out the unsubscribe thing - it's mapped to the 'stopped' action type and I was still looking for 'unsubscribe'. I too love the use of promises for the reasons you stated and because they can be chained if needed.

One other thing I haven't quite yet figured out though is what these should look like in the store as far as the middleware portion. The old one(s) had something like meteorMethod(newSuccessNotification, newErrorNotification), but that doesn't apply anymore and returns TypeError: next is not a function if you run your new stuff with that. How should these look in the store middleware section?

I'm sure I'll figure it out if I play a bit more, but if you have a sec to clarify this, that would be cool.

dbismut commented 8 years ago

Clearly I'm not using notifications in my implementation just yet. To be honest, I'm not sure that's really relevant the way it's done in the original implementation. Back to the original implementation code, I would rather do:

export default store => next => action => {
  if (!action.meteor || !action.meteor.call) {
    return next(action);
  }

  const { method, parameters, onSuccess, onError } = action.meteor.call;
  const params = parameters || [];

  Meteor.call(method, ...params, (error, result) => {
    if (error) {
      if (onError) {
        return onError(error);
      }
      return next({type: NEW_NOTIFICATION, level: 'danger', message: 'error message'});
    }

    if (onSuccess) {
      return onSuccess(result);
    }
      return next({type: NEW_NOTIFICATION, level: 'success', message: 'success message'});
  });
};

...of course, that would mean you would combine the middlewares as in:

const middlewares = [
  thunkMiddleware,
  meteorSubscription,
  meteorDatasource,
  meteorMethod,
  meteorInsert,
  meteorUpdate,
  meteorRemove,
  loggerMiddleware,
];

Hope this makes sense?

genyded commented 8 years ago

OK - i took out the notification stuff altogether, but am still getting TypeError: next is not a function from your meteorMethod call. I also noticed that your not literally dispatching your actions...

export function signIn(email, password) {
  return {
    type: USER_SIGNIN,
    meteor: {
      call: {
        method: Meteor.loginWithPassword,
        params: [email, password]
      }
    }
  }
}

...as opposed to...

export function signIn(email, password) {
  return dispatch => {
    dispatch({
        type: USER_SIGNIN,
        meteor: {
          call: {
            method: Meteor.loginWithPassword,
            params: [email, password]
          }
        }
    })
  }
}

How can they work with dispatch?

dbismut commented 8 years ago

Well, the first action without the dispatch is what an action is supposed to look like: you still need to dispatch it as in dispatch(signIn), re-read the redux tutorial if you need to :)

The structure with dispatch => dispatch is for thunks, but in that situation you would not need any :) In the current code base from the nutrition app, there is a lot of useless thunks that I stripped out.

Can you please paste your meteorMethod middleware? Does it start with:

export default store => next => action => { ... }

Can you also paste how you combine middlewares? Make sure you don't add the meteorMethod middleware by calling the function meteorMethod()...!

genyded commented 8 years ago

Here is the middleware and it behaves the same with or without dispatch...

meteorMethod.js

import actionTypeBuilder from '../ducks/actionTypeBuilder';

export default store => next => action => {
  if (!action.meteor || !action.meteor.call) {
    return next(action);
  }

  const { method, params } = action.meteor.call;
  const parameters = params || [];

  const meteorMethod = typeof method === 'string' ? Meteor.call : method;

  if (typeof method === 'string') {
    parameters.unshift(method);
  }

  return new Promise((resolve, reject) => {

    next({ type: actionTypeBuilder.loading(action.type) });

    meteorMethod(...parameters, (error, result) => {
      if (error) {
        next({ type: actionTypeBuilder.error(action.type), error });
        return reject(error);
      }

      next({ type: actionTypeBuilder.success(action.type) });
      return resolve(result);
    });
  });
};

Here is the combine...

...
const middleware = [
  thunkMiddleware,
  routingMiddleware,
  meteorSubscription,
  meteorDatasource,
  meteorMethod,
  //meteorMethod(newSuccessNotification, newErrorNotification),
  //meteorInsert(newSuccessNotification, newErrorNotification),
  //meteorUpdate(newSuccessNotification, newErrorNotification),
  //meteorRemove(newSuccessNotification, newErrorNotification),
  //loggerMiddleware
];

const finalCreateStore = compose(
  applyMiddleware(...middleware),

...
dbismut commented 8 years ago

Hmm... I don't know, that's weird. There is this issue mentioning something similar: https://github.com/gaearon/redux-thunk/issues/15

... but I guess that's not it right?

dbismut commented 8 years ago

@genyded not sure if this is related, but there was a bug with the way I handled unsubscribe in meteorSubscribptions. I updated the gist with a fix.

you-fail-me commented 7 years ago

sorry for necroposting, but I'm thinking about making front end of a meteor app easier to live with, and I consider adopting redux (maybe for just localstate management, maybe for data from mongo as well, not sure yet). I find what you post here really nice and promising, and would like to ask, what happened to all this now, after a year passed? were you happy with this solution? has it evolved to maybe some npm package I could use? how ready for prod use is stuff in the gist by @dbismut? would you advise to use it? or maybe you've got totally disappointed with this concept and it's not really worth that? thanks

you-fail-me commented 7 years ago

Btw, is there a way to have some branch of the store as a reactive data source, so I could dispatch an action that would e.g. flip some flag in it and all the components that have that flag as a prop would automatically re-render?

dbismut commented 7 years ago

Hey @you-fail-me, honestly this was done a long time ago, and even if this still works with the packages at the time, I wouldn't be so sure with the latest updates (Meteor 1.4, React 15, etc.). I'm using it in production but I wouldn't consider it very stable. I don't remember exactly, but I know there are some edge cases where everything doesn't work so well. Also, I'm not a pro developer, so anything I do I wouldn't recommend in production ;)

However, I've updated the gist with the latest code I wrote last year if you want to have a look. But considering all the controversy around Meteor, I'm not sure I would advise pursuing this route.

you-fail-me commented 7 years ago

@dbismut Thank you for reply! What do you guys think about this https://github.com/samybob1/meteor-redux-middlewares ?

djhi commented 7 years ago

Hi @you-fail-me, I don't use Meteor anymore and won't be working on this projet in this form. I might rebuild it with the stack I use for almost 2 years now which I have revisited recently with Apollo.

My current stack uses:

No real time updates for now but at least you're free to do it your way. Too much lock in with Meteor.