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

Support async/await & promises #81

Closed
one000mph opened this issue Oct 1, 2018 · 16 comments
Closed

Support async/await & promises #81

one000mph opened this issue Oct 1, 2018 · 16 comments

Comments

@one000mph
Copy link

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
Copy link
Contributor

tot-ra commented Nov 19, 2018

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

@MelMacaluso
Copy link

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

@one000mph
Copy link
Author

one000mph commented Sep 11, 2019

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
Copy link

@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
Copy link

enzoferey commented Oct 23, 2019

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
Copy link

kopax commented Oct 30, 2019

@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.

@tot-ra tot-ra changed the title Any plans to update this to support async/await? Support async/await & promises Nov 1, 2019
@one000mph
Copy link
Author

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
Copy link

kopax commented Nov 1, 2019

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
Copy link
Contributor

tot-ra commented Nov 4, 2019

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
Copy link

MelMacaluso commented Feb 14, 2020

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
Copy link

kopax commented Feb 15, 2020

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

@enzoferey
Copy link

enzoferey commented Feb 15, 2020

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

@kopax
Copy link

kopax commented Feb 15, 2020

@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 #81 (comment)

@RuTsEST RuTsEST closed this as completed Feb 28, 2020
@RuTsEST RuTsEST reopened this Feb 28, 2020
@RuTsEST
Copy link
Contributor

RuTsEST commented Feb 28, 2020

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

@RuTsEST RuTsEST closed this as completed Feb 28, 2020
@kopax
Copy link

kopax commented Feb 28, 2020

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
Copy link

Thanks guys! Now buckle up your seatbelts is refactor time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants