Skip to content
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

Better support for promises #196

Open
jpmonette opened this issue Aug 4, 2020 · 3 comments · May be fixed by #221
Open

Better support for promises #196

jpmonette opened this issue Aug 4, 2020 · 3 comments · May be fixed by #221

Comments

@jpmonette
Copy link
Contributor

It would be great to support promises without having to rely on the promiser, in order to:

  • simplify typing (especially if there is a shift to TypeScript codebase);
  • support functions that have a different order of arguments (such as sendRequest, where successCB and errorCB are not the last arguments)
export const sendRequest = <T>(
  endPoint: string,
  path: string,
  successCB: ExecSuccessCallback<T>,
  errorCB: ExecErrorCallback,
  method?: HttpMethod,
  payload?: Record<string, unknown> | null,
  headerParams?: Record<string, unknown> | null,
  fileParams?: unknown,
  returnBinary?: boolean,
  doesNotRequireAuthentication?: boolean,
): void => {

This would require to add a type check in each function to see if the callbacks have been provided. If not, the function could return a Promise.

Here's a sample implementation for the query function:

type QueryOverload = {
  /**
   * Executes the specified SOQL query.
   * @param soql a string containing the query to execute - e.g. "SELECT Id,
   *             Name from Account ORDER BY Name LIMIT 20"
   * @param callback function to which response will be passed
   * @param [error=null] function called in case of error
   * @returns void
   */
  <T>(soql: string, successCB: ExecSuccessCallback<T>, errorCB: ExecErrorCallback): void;
  /**
   * Executes the specified SOQL query.
   * @param soql a string containing the query to execute - e.g. "SELECT Id,
   *             Name from Account ORDER BY Name LIMIT 20"
   * @returns Promise<T>
   */
  <T>(soql: string): Promise<T>;
};

export const query: QueryOverload = <T>(soql: string, successCB?: ExecSuccessCallback<T>, errorCB?: ExecErrorCallback): any => {
  if (successCB && errorCB) {
    sendRequest("/services/data", `/${apiVersion}/query`, successCB, errorCB, "GET", { q: soql });
  } else {
    return new Promise((resolve, reject) => {
      const successCallback = (results: T) => resolve(results);
      const errorCallback: ExecErrorCallback = (error) => reject(error);
      sendRequest("/services/data", `/${apiVersion}/query`, successCallback, errorCallback, "GET", { q: soql });
    });
  }
};

query("SELECT Id FROM Account")
  .then((results) => console.log(results))
  .catch((err) => console.error(err));

query(
  "SELECT Id FROM Account",
  (results) => console.log(results),
  (err) => console.error(err),
);

And of course, we would benefit from the IntelliSense for both versions of the function.

Capture d’écran, le 2020-08-04 à 11 06 23

@wmathurin
Copy link
Contributor

We have a "promiser" helper function that allows you to turn any our callback based APIs into promise based APIs with a single line of code.

For an example of the promiser in action see: https://github.com/forcedotcom/SalesforceMobileSDK-Templates/blob/dev/MobileSyncExplorerReactNative/js/StoreMgr.js#L30

@jpmonette
Copy link
Contributor Author

jpmonette commented Jan 8, 2021

@wmathurin As mentioned above, here are the limitations with promiser:

  • This does not work for all methods (sendRequest is a good example of a method that breaks with promiser, because the order of parameters is inconsistent)
  • It completely strips the types off when used in TypeScript, as promiser cannot predict the future type of the function created.

@wmathurin
Copy link
Contributor

Valid points @jpmonette. Re-opening issue.

@wmathurin wmathurin reopened this Jan 8, 2021
@jpmonette jpmonette linked a pull request Feb 17, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants