-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update actions to rely on a single action.reject with throw messages for individual methods #50
Comments
@tafelito the second method shouldn't be firing if the |
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. |
@tafelito okay, I'll have to take a look and run some experiments. |
ok great! let me know if I can be of any help |
Took a look at this the other day with a customer having a similar issue. What I found was that, by
Subsequent steps are blocked as expected. So, the big fix seems to be instead of calling 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. |
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 |
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 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. |
@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. |
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 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? |
@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 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
} |
Hey @tafelito how does the top code block with the |
@cleverbeagle you import it on
|
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. |
In your docs says, when executing an action
Is that true? Because as per my understanding, JS will execute all remaining code, if any, before actually returning the promise value.
So if I have a reject and then I call another action, it will run the 2nd action even if the 1st one has been rejected
What would be the best way to prevent further execution after a promise has been rejected?
Thanks
The text was updated successfully, but these errors were encountered: