kadirahq / mantra

Mantra - An Application Architecture for Meteor
https://kadirahq.github.io/mantra/
977 stars 51 forks source link

Calling an action inside another action #107

Open lsunsi opened 8 years ago

lsunsi commented 8 years ago

Is this a bad practice or even allowed in Mantra? If it is, what is the pattern for doing that?

My current context is that a have a newNotification action and I wanna call it if the Meteor.login call fails. I'm having problems thinking of a way of achieving this without relying on the component (which should be logic-less according to mantra)

arunoda commented 8 years ago

We discussed on this here: https://forums.meteor.com/t/mantra-calling-actions-from-other-actions/18135

arunoda commented 8 years ago

Rethink about how to do this and update the spec.

lsunsi commented 8 years ago

@arunoda thanks for reopening this thread. If I come up with something I'll post it here. Would passing actions as a dependency to an action be such a bad idea?

arunoda commented 8 years ago

If we allows that, actions may depend on some other modules actions. But we need to read the code to identify that. If it's the import, we define it explicitly.

So, with the action calling, if we remove a module we may get runtime errors as other modules using it inside an action. But if we import a common function, we know we can it.

BTW: there are some other places, we use actions. So, my argument on removing module may not be correct 100%.

lsunsi commented 8 years ago

@arunoda Agreed, so making all actions available with actions would mean implicit dependency, which is what we want to avoid. Just importing the required module's actions fix that problem, but creates another one because it'd be a non-injected dependency.

Should an action be able to call another action that is declared alongside itself? I mean, when you export and object with functions, every function is available to every other one with 'this'. If you explicitly imported another module's actions and included in your exported object it'd be accessible via 'this' and testable via 'bind'.

This isn't actual injection, but... I don't know. Can't think of anything better. EDIT: Some actions would be duplicated in app.actions following this logic, so.. yeah. Not nearly an ideal solution.

luisherranz commented 8 years ago

You need a notification system to decouple your actions from your logic. You can use the Flux dispatcher, for example. Take a look at async actions. It will look something like this:

// in module1 LoginButton.jsx
const LoginButton = () => (
  <button onClick={() => Dispatcher.dispatch({ type: 'LOGIN_REQUEST', user: .., pass: .. })}>
      Login!
  </button>
);

// in module1 someLogicFile.js
Dispatcher.register(action => {
  if (action.type === 'LOGIN_REQUEST')
    Meteor.login(action.user, action.pass, function(err) {
      if (err)
        Dispatcher.dispatch({ type: 'LOGIN_FAILURE', error: err });
      else
        Dispatcher.dispatch({ type: 'LOGIN_SUCCESS' });
    });
});

// in module2 someOtherLogicFile.js
Dispatcher.register(action => {
  if (action.type === 'LOGIN_FAILURE')
    do something;
});

If you remove module2 nothing breaks.

lsunsi commented 8 years ago

@luisherranz That is a cool solution, although it requires more than just Mantra to achieve, right?

Also, I am using Redux for local state management, which is not exactly 'flux' since if doesn't include a dispatcher. Could you make a similar example of how should I make this work in a Redux environment?

luisherranz commented 8 years ago

Yes, as far as I know Mantra doesn't include anything like this by default.

Redux is inspired by Flux, although sometimes it has a more complex API and it needs the use of external middleware. I exposed the Flux example for simplicity, but Redux does have a dispatcher as well.

To achieve this type of async actions in Redux, you need to use a middleware called thunk: https://github.com/gaearon/redux-thunk Action functions using thunk return other functions (instead of objects) which can dispatch new actions.

lsunsi commented 8 years ago

@luisherranz Yeah, that's cool actually. I'm reading about and it does seem overly complicated, but I'll keep at it. I just started writing apps with 'flux' like state management, and redux was my first pick (it seemed more solid). Would you recommend some other flux implementation?

luisherranz commented 8 years ago

Yes, right now Redux is probably the best choice. Even people from Facebook seem to be using it.

In my opinion it has some great things, but the API is over complicated and it needs too much middleware for normal stuff. A better state management solution will arise for sure.

lsunsi commented 8 years ago

@luisherranz Thanks for all your help man. I'll check out redux-thunk and try to make this work. To problem is that I'd need a thunk every time I wanted to show a notification. Well

zimt28 commented 8 years ago

@luisherranz Maybe @contra's shasta might help, but I don't know if he's still working on it.

yocontra commented 8 years ago

@zimt28 Yes, I am working on it but it's a large project and I am only one person so it's going to take me a minute to finish it.

luisherranz commented 8 years ago

@contra is it a new API for Redux or a template to get started?

yocontra commented 8 years ago

@luisherranz New API on top of redux

zimt28 commented 8 years ago

@contra Thanks for letting us know, looking forward to it :+1:

lsunsi commented 8 years ago

@luisherranz Actually the 'redux-thunk' we talked about doesn't seem to decouple the actions like your 'flux' example, if I'm not mistaken. I mean, I'd still have to call both actions in the same thunk action creator, meaning they would have to be imported in some module.

Am I missing something?

lsunsi commented 8 years ago

@arunoda Using mantra-core, all actions are loaded first via loadModule and are store alongside the app context to be passed down to components via containers. Doesn't that mean a container can be implicitly dependent on an action some other module exported? Meaning the container can pass down to a component an action from another module without explicitly requiring this other module.

arunoda commented 8 years ago

@lsunsi yeah. That's how it works.

luisherranz commented 8 years ago

I'd still have to call both actions in the same thunk action creator, meaning they would have to be imported in some module.

@lsunsi your are absolutely right. I forgot about that, sorry. For me that's one of the pitfalls of Redux, state management is after the dispatcher but side-effects are before. It makes no sense.

This is off-topic (sorry @arunoda, I know you don't like it) but I'd like to bring up something here: A while ago I wrote a proposal of my own version of Redux called Trux. It's based on a TRP library called mobservable which has several improvements over Tracker. One of the principles of Trux was: every bit of logic goes after the dispatcher. Actions can't have any logic, not even deal with async operations. Modularity and extensibility was one of its core principles. Since then I've kept working on other things but thinking along side about Trux. I have changed the API and thought about many use cases which now start to feel natural to write.

Mobservable reached 2.0 a couple of days ago and changed it's name to Mobx. Now you can create your own reactive libraries on top of it, like we used to do with Tracker so I think it may be a good time to rewrite the proposal with the new API and let it out for some criticism and discussion. I want to see if people would be interested in something like this.

Just for the record, this is how you would do this on Trux with the new API. It's heavily based on ES6 arrow functions and object destructuring. Everything is functional and uses dependency injection for easy testing and mocking. I got rid of other bad Redux/Flux things, like the need for strings. Although I am using a TRP library, every dependency is explicitly declared. Exports are for testing purposes only. It not only has a single object tree for state, but for actions, reactions and components as well. It has only one function: connect which connects everything for you so boilerplate code goes to near zero. Actions and state are observables, which mean any part of the system can be triggered by them (pretty much like we were used with Tracker).

// in accounts/actions.js
export const loginRequest = (username, password) => ({ username, password });
export const loginSuccess = () => ({});
export const loginFailure = (error) => ({ error });
connect({ actions: { accounts: { loginRequest, loginSuccess, loginFailure }}});

// in accounts/components/LoginButton.jsx
export const LoginButton = ({ loginRequest }) => (
  <button onClick={() => loginRequest(user: .., pass: .. )}> Login!</button>
);
LoginButton.props = ({ actions }) => ({ loginRequest: actions.accounts.loginRequest });
connect({ components: { accounts: { LoginButton }}});

// in accounts/reactions.js
export const loginHandle =  ({ action, loginSuccess, loginFailure, login }) => {
  login(action.username, action.pass, error => {
    if (error) loginFailure(error);
    else loginSuccess();
  });
};
loginHandle.actions = actions => actions.accounts.loginRequest;
loginHandle.props = ({ actions, injections }) => ({
  loginSuccess: actions.accounts.loginSuccess,
  loginFailure: actions.accounts.loginFailure,
  login: injections.meteor.login
});
connect({ reactions: { loginHandle }});

// in other-module/reactions.js
export const doSomething = ({ action }) => {
  do something;
};
doSomething.actions = actions => actions.accounts.loginFailure;
connect({ reactions: { doSomething }});

Reactions are for side-effects but this is how state management would work:

// in accounts/state.js
export const someNumber = ({ state, action, otherNumber, someMath }) => {
  return someMath(state, action.someNumber + otherNumber);
};
someNumber.actions = actions => ([ actions.someAction, actions.someOtherAction ]);
someNumber.props = ({ state, injections }) => ({
  otherNumber: state.otherNumber,
  someMath: injections.myMathLib.something
});
connect({ state: { someNested: { someNumber }}});

And this is a dependent state modifier. It's similar to Tracker's autorun, but with explicit dependencies.

export const dependetNumber = ({ someNumber }) => {
  return someNumber + 1;
};
dependetNumber.deps = ({ state }) => ({ someNumber: state.someNested.someNumber });
connect({ state: { dependentNumber }});

You can mix anything (state dependencies, props, actions) together in state modifiers or reactions.

I'll try to rewrite the proposal this weekend including use cases for things like routing, validations, async ops, i18n...

If somebody is interested and have questions or suggestions open an issue in the trux repo, so we don't hijack this issue (sorry again @arunoda). Just as a reminder, the actual proposal in that Readme is not updated to the latest changes.

lsunsi commented 8 years ago

@luisherranz That's pretty interesting but kind of complicated from the first look. I'm gonna read more about it

lsunsi commented 8 years ago

@arunoda and is that how it should work? I mean, not having implicit dependencies is the whole reason for not passing actions as an argument for an action, right?

@luisherranz what do you think about man? (:

arunoda commented 8 years ago

@lsunsi I think it's safe to allow actions to call other actions. I'll update the spec on this.

lsunsi commented 8 years ago

@arunoda but to do that we'd need to explicitly import the action, right? or are you going to update the dependency injection package so that it passes the actions as a second argument?

arunoda commented 8 years ago

@lsunsi yes. something like that.

lsunsi commented 8 years ago

@arunoda Since injecting the actions as the second argument passed to actions was pretty easy, I submitted a pull request making that change in the react-simple-di repo. I don't know if this is the best way of achieving this functionality, but I'm just trying to help (:

chmanie commented 8 years ago

I love the idea and would support getting forward with this. Commented here: https://github.com/kadirahq/react-simple-di/pull/3

adamdawkins commented 8 years ago

+1