mvoronin80 / AuraPromise

This is a simple implementation of Promises support for Aura framework
MIT License
14 stars 5 forks source link

access check issue and anti-patterns #1

Open ghost opened 8 years ago

ghost commented 8 years ago

I saw the recording of your DF16 presentation (https://www.salesforce.com/video/304574/). It's great content and this repro has some good patterns. We recently published documentation about using promises with Lightning: https://developer.salesforce.com/docs/atlas.en-us.lightning.meta/lightning/js_promises.htm

However I saw some issues with the patterns and code I wanted to bring to your attention.

  1. Promises execute in a microtask which, by definition, breaks out of the Lightning event loop and the current Lightning access context. If you're unfamiliar with microtasks please read https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/.

If you need to preserve access context or need to reenter the Lightning event loop use $A.getCallback(). Eg

getAPromise().then(
  $A.getCallback(function resolve(value) { /* resolve handler */ }),
  $A.getCallback(function reject(error) { /* reject handler */ })
)

You need to be within the Lightning event loop when you enqueue an action (and you need to be within an access context to create the action in the first place). Eg

getAPromise().then(
  $A.getCallback(function resolve(value) { 
    var action = cmp.get("c.method");
    action.setParams({...});
    action.setCallback(scopeVar, function(response) {
      /* check response.getState() and handle accordingly */
    });
    $A.enqueueAction(action);
  }),
  $A.getCallback(function reject(error) { /* reject handler */ })
)

You can learn more about $A.getCallback() at https://developer.salesforce.com/docs/atlas.en-us.lightning.meta/lightning/js_cb_mod_ext_js.htm.

A large part of https://github.com/mvoronin80/AuraPromise/blob/master/src/staticresources/AuraPromise.resource requires updating to reflect this as do your 3 sample components.

  1. Always include a reject handler or catch in your promise chain. Your code appears to do this but you may want to use $A.reportError() to handle non-recoverable errors. Throwing an error in a promise will not trigger window.onerror, which is where Lightning hooks up the global error handler. Watch the JS console for reports about uncaught errors in a promise to verify you've done it correctly.

  2. It is an anti-pattern to create a waterfall of actions for two reasons: a. It is non-performant. Chaining actions together via promise.then.then creates multiple round trips to the server. That is not fast especially on mobile devices which often suffer from high latency connections. b. The entire flow of actions is not transactional. Each action is run within a transaction but a set of actions are spread across transactions. Your examples were read-only but if any were to perform mutating operations then it'd be an issue.

  3. Promises are incompatible with storable actions (https://developer.salesforce.com/docs/atlas.en-us.lightning.meta/lightning/controllers_server_storable_actions.htm). Storable actions may have their callbacks invoked more than once. This models the inherent mutable nature of Salesforce metadata and data. It's more like streams / reactive programming than most realize. This doesn't align well with the promise flow of 1-time resolve/reject state transition and thus callbacks are used in action.setCallback(scope, callbackFunction).

  4. Aura Actions should always handle all response states (success, error, and incomplete states). The sample component code does this some of the time but not all the time. AuraPromise (https://github.com/mvoronin80/AuraPromise/blob/master/src/staticresources/AuraPromise.resource#L22) fails to handle incomplete state. It'd be better to structure things as an if (success) {} else { failure } to catch all states.

kevinv11n commented 8 years ago

Doing some account merging so please reach me at this account.

mvoronin80 commented 8 years ago

Thanks for your feedback, Kevin. I didn't have much time to reply to your comment right now, but I agree with most of them. I will update my code in about a week to reflect most of your concerns. Again, thank you very much, the feedback is really helpful.