Update actions to rely on a single action.reject with throw messages for individual methods about pup HOT 13 CLOSED

@tafelito okay, I'll have to take a look and run some experiments.

from pup.

@cleverbeagle you import it on methods.js

import doAction from './do-action';

from pup.

@tafelito the second method shouldn't be firing if the reject() is called as far as I know. If it is then this will need to be revised. Do you get the error from the first method action1 as expected back on the client?

from pup.

Yes you get the error from the client, but action2 is still executed. I understand that's how Promises work, that's why I wasn't exactly sure. The only way you can finish the execution is you throw an error or if you actually return a reject. But in this case, because you are catching the error within the action, it will actually return to where it was called originally. So it goes back to the handler.

from pup.

ok great! let me know if I can be of any help

from pup.

Took a look at this the other day with a customer having a similar issue. What I found was that, by throwing in the code like this:

/* eslint-disable consistent-return */

import { Meteor } from 'meteor/meteor';

let action;

const anotherMethod = () => {
  try {
    console.log('Another message we should not see.');
  } catch (exception) {
    throw new Error(`[editProfile.anotherMethod] ${exception.message}`)
  }
};

const updateUser = (userId, { emailAddress, profile }) => {
  try {
    throw new Error('Something went wrong.');
    Meteor.users.update(userId, {
      $set: {
        'emails.0.address': emailAddress,
        profile,
      },
    });
  } catch (exception) {
    throw new Error(`[editProfile.updateUser] ${exception.message}`);
  }
};

const editProfile = ({ userId, profile }, promise) => {
  try {
    action = promise;
    updateUser(userId, profile);
    anotherMethod();
    action.resolve();
  } catch (exception) {
    action.reject(exception.message);
  }
};

export default options =>
new Promise((resolve, reject) =>
editProfile(options, { resolve, reject }));

Subsequent steps are blocked as expected. So, the big fix seems to be instead of calling action.reject() in each additional method outside the handler, just throw the error and rely on the main handler function (in this case editProfile) to reject the action. Also, notice that I'm using a vanilla JS Error and not Meteor.Error (this is because the purpose of Meteor.Error is to get an error back to the client from within a Meteor Method—we still keep that in the .catch() block of the original call to our action).

A quick example of running this gets what we'd expect:

Going to flag this as a refactor and update the docs :) Thanks for the poke to check it out.

from pup.

Great!

The only thing I'm not fully convinced is the use of a Promise. You can actually use the same structure without Promises and just using try/catch blocks.

Also I noticed that using it this way, you are missing the opportunity to throw custom error codes. Usually when you throw an error to the client, on the client side you catch it with the error code, so you show the appropriate message for that specific code

from pup.

Hi @tafelito coming back to this, I think you're right. In theory, it's the code inside of the action that needs to concern itself with async stuff, not the action itself. So, if there was a suggestion that you use async/await where appropriate, you could get the same end result.

My one hesitation, though, is that having access to the resolve/reject and then/catch callbacks makes value and error handling a bit more obvious. No technical implications here, just making the pattern easier for a beginner to follow.

Any further thoughts? Just came to mind that it may be worth putting together a lil' spec for these things to better codify the pattern.

from pup.

@cleverbeagle I'm not sure if I'm following the async/await part you mentioned. I'm still not sure what are the benefits using promises here. To me, it's actually harder to understand why you need some async code just to execute sync actions. I don't see any extra benefits from it. But that's just me.

Another problem I see using promises within the actions, is when you have to share some of this actions somewhere else. So let's say that the updateUser action in edit-profile.js is actually shared between other methods/actions as well. So instead of declaring in edit-profile.js, you import it from modules. Now you have to send the action as parameter so updateUser can resolve or reject the promise, where if you just throw an exception, you just need to catch it whenever you call that action.
And try/catch is widely used, so I'm sure people is familiar with it, or maybe not (but if they don't, they definitely won't know promises either 😜)

from pup.

Hey @tafelito sorry for the 🐢

Re: usage of Promises, really it comes down to judgment. You may need them, you may not. I typically default to using them in actions as they end up being used in relation to third-party code. That said, nothing wrong with just using regular try/catch patterns and throw statements.

In respect to sharing functions, if this is the case, typically I'll just take the given method and make it a standalone function that gets imported into the action file (or wherever else it's required). This is another judgment call as it depends on how often the code will be reused—for example, if just twice, a bit of repetition is minimal.

Curious, if you were to write out an action, what would it ideally look like?

from pup.

@cleverbeagle this is how I usually do it. My background is Java though, so I guess that's why I'm more use to this way, not only for meteor.

methods.js

method() {
    // Check if user is logged in, in case of authorized methods only. It actually would be better to have some sort of, authorized methods and avoid checking this every time
    if (!this.userId) throw new Meteor.Error(ERRORCODES.NOT_AUTHORIZED, 'not-authorized');

    doAction();
},

do-action.js

export default ({  }) => {
    // Having multiple actions
    callFirstAction();
    callSecondAction();

    // Or just do the action here if not to complex

    // Thrown exceptions are going straight to the client but if you wanna continue with execution, just use try/catch for those cases, like
    try {
        callFirstAction();
    } catch () {
        // maybe print the error or do something with it
    }
    // continue execution
}

This could be on the same do-action file or in a different file maybe in /modules
first-action.js

export default ({  }) => {
    // if known error, throw either Meteor.error or just Error (depending on the type of error)
    // use try/catch to handle unknown errors and throw it with a better description
}

from pup.

Hey @tafelito how does the top code block with the method() snippet in it relate back to the bottom two?

from pup.

I'll have to give this a think! There's definitely something here. Could see quite a few variations of the pattern emerging depending on context.

from pup.