pipedrive / client-nodejs

Pipedrive API client for NodeJS
MIT License
205 stars 82 forks source link

Support async/await & promises #81

Closed one000mph closed 4 years ago

one000mph commented 6 years ago

I'd like to use this in a node 10 app with async/await, but as is I would need to use callback syntax.

tot-ra commented 5 years ago

This lib would need a significant rewrite to support that. @ziimk and team is considering that, no ETAs or roadmap yet

MelMacaluso commented 5 years ago

@tot-ra any news on the async/wait version? 😢

one000mph commented 5 years ago

I'm currently using async/await calls with this library with a simple wrapper function. It's pretty easy to transform a callback format with the existing Promiseobject class. Hope this helps others who are still "awaiting" 😆 async/await

My api framework is hapi so logging happens with using request.log

## lib/wrappers/pipedrive.js
const OriginalPipedrive = require('pipedrive');
const Config = require('getconfig');

const { isDev } = Config.getconfig;
const apiToken = Config.pipedrive.token;

const Pipedrive = function (token) {

  if (!isDev) {
    this.pipedrive = new OriginalPipedrive.Client(token, { strictMode: true });
  }
};

Pipedrive.prototype.getPerson = function (id, request) {

  if (this.pipedrive) {

    return new Promise((resolve, reject)  => {

      return this.pipedrive.Persons.get(id, (err, person) => {

        if (err !== null) {
          request.log(['error', 'pipedrive'], 'Could not get Person: ' + id);
          reject(err);
          return;
        }

        resolve(person);
        return;
      });
    });
  }
};

In another file

const Pipedrive = require('./lib/wrappers/pipedrive');

## inside handler so we have `request` defined
const person = await Pipedrive.getPerson(id, request);
MelMacaluso commented 5 years ago

@one000mph thanks a lot for the wrapper indeed, I like it. Still OTB so we can avoid writing that in our codebase would be even cooler ahah.

enzoferey commented 4 years ago

If somebody is looking for a decoupled version of what @one000mph suggested:

function promiseWrapper(apiCall, payload) {
  return new Promise((resolve, reject) => {
    return apiCall(payload, (error, resource) => {
      if (error !== null) {
        reject(error);
        return;
      }
      resolve(resource);
      return;
    });
  });
}

You can later use it as:

const dealPayload = { ... };
const createdDeal = await promiseWrapper(PipedriveClient.Deals.add, dealPayload);

It is a bit verbose, but avoids having to declare prototypes overrides all over the place 😄

kopax commented 4 years ago

@tot-ra I don't think it is much work, wrapping a cb in a Promise is a well-known operation, for example with setTimeout.

Can @one000mph solution be added in this repository, we can just keep have both available for each method.

I don't like the @enzoferey wrapper as it requires us to think more, to give more explanations. If this is not added in this core then I'll probably use it as it is but that won't look nice.

one000mph commented 4 years ago

I would have some time in the next week or so to add my wrapper to this lib if that's something you wanted to pursue @tot-ra. I haven't contributed to this repo before so lmk if there's any rules or conventions I should look at.

kopax commented 4 years ago

Thanks @one000mph that's a good move for JS developer who want to use pipedrive.

You should maybe try to contact them by email first if you want to get a fast answer, they look not to check here often.

I will have a development that include pipedrive integration in the next few weeks, in case you do it before can I please subscribe to the repo to see the development progress, even if it's not integrated on this official branch, I may use it if it's released on time for my development, otherwise, I'll pick the cb version and upgrade later.

tot-ra commented 4 years ago

My vision is to have library support promises natively, without using callbacks (so it would be a breaking change with major release), that's why I wrote that it would need a significant rewrite. Sure, you can use wrappers for now.

If you make a PR and don't get a review in a timely manner, you can:

MelMacaluso commented 4 years ago

To be honest with you guys I like @enzoferey version more, straight to the point and usable in most cases...

There's an edge case which needs a small rewrite (when invoking a method from ie. pipedrive.Deals.get response such as: deal.addFollowers() because for some unknown reason (flaw in the wrapper design?) it takes an id and a payload 😂 .

A small verbose quick rewrite to handle also that edge case would be:

const promiseWrapper = (apiCall, id, payload) => {
  return new Promise((resolve, reject) => {
    if(id) {
      return apiCall(id, payload, (error, resource) => {
        if (error) {
          reject(error)
        }
        resolve(resource)
      })
    }
    return apiCall(payload, (error, resource) => {
      if (error) {
        reject(error)
      }
      resolve(resource)
    })
  })
}
kopax commented 4 years ago

If someone publishes a community library that works with async/await until Stripe implements it, let us know here.

enzoferey commented 4 years ago

Is Stripe working on an implementation at the moment ? How long do you think it will take them to release it ?

kopax commented 4 years ago

@enzoferey they currently does not seems to want to support the feature at the moment. You can try to contact them and ask your question using their communication channel, see https://github.com/pipedrive/client-nodejs/issues/81#issuecomment-549260130

RuTsEST commented 4 years ago

Async/await is now supported as of version 10. #111

kopax commented 4 years ago

Wow excellent thanks a lot @RuTest that was unexpected and quick. Thanks for approving this feature request. I am still looking to integrate pipedrive with react-native components (such as react-native-paper, it would be great if you could also help us there. Should I open a new ticket?

MelMacaluso commented 4 years ago

Thanks guys! Now buckle up your seatbelts is refactor time