pipedrive / client-nodejs

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

Why input payload is an array instead of an object? #120

Closed MelMacaluso closed 4 years ago

MelMacaluso commented 4 years ago

I am probably missing some logic behind it or similar but the input payload seems to be required to be an indexed array?

Instead of a regular object as usually, API design requires (and that will make the code much cleaner instead of creating an empty array and then assigning props through bracket notation...).

Am I missing something?

Say your suggested:

  var input = [];
        input['userId'] = 58;
        input['filterId'] = 58;
        input['stageId'] = 58;
        input['status'] = new Status2Enum(all_not_deleted);
        input['start'] = 0;
        input['limit'] = 58;
        input['sort'] = 'sort';
        input['ownedByYou'] = Object.keys(NumberBoolean)[0];

    controller.getAllDeals(input, function(error, response, context) {
      ...
    });

Wouldn't it be much more readable/easy to work with the following simpler approach?

  const input = {
    userId: 58,
    filterId: 58,
    stageId: 58,
    status: new Status2Enum(all_not_deleted),
    start: 0,
    limit: 58,
    sort: 'sort',
    ownedByYou: Object.keys(NumberBoolean)[0]
  }

    controller.getAllDeals(input, function(error, response, context) {
      ...
    })
RuTsEST commented 4 years ago

Hi @MelMacaluso! Great question! You can either an indexed array or a regular object, they are pretty much interchangeable. I agree that it would be more convenient to have objects in our docs but unfortunately this is the way it was generated from out API specs and can not be easily changed. You can have a look at how we used the Controllers/methods in our examples folder.

MelMacaluso commented 4 years ago

Hi @RuTsEST , thanks for the answer!

After playing around with it it seems that's the way to say addAPerson

  const input = {
    body: {
      name: 'something',
      email: 'something@test.com',
      phone: '123',
    },
  }

Thing is, where in the documentation does it say that the payload needs to be the value of a body prop of an object? I had to deduce it, maybe it should be added somewhere?

Btw I also tried the suggested way and it doesn't work (while the one above does)

  const input = []
  input['name'] = 'something'
  input['email'] = 'something@test.com'
  input['phone'] = '123'
kkudose commented 4 years ago

@MelMacaluso I agree the documentation should be improved. For that particular case, it is noted here: https://github.com/pipedrive/client-nodejs#parameters-113

MelMacaluso commented 4 years ago

@kkudose Thanks, yes they could be greatly improved as one of the main things is sending a payload and that specific field has "TODO description" Ahah.

Anyways I am really enjoying the new rewrite and got ride of so much boilerplate I was using.

Out of curiosity that specific example, isn't it wrong/misleading? (https://github.com/pipedrive/client-nodejs#example-usage-120) as they hint you can pass an id in the body to add a person? As far as I know, id is not valid as ids for persons/deals are created by PD itself?

RuTsEST commented 4 years ago

Yes, you are absolutely correct! I'll investigate why the SDK has an id in the example even though in our OpenAPI3 spec there is none. 🤔

MelMacaluso commented 4 years ago

@RuTsEST I think you guys did a great job in v10, especially the supported await. Still, if I can say anything from someone refactoring all the codebase from the previous version, many things are left unsaid or you need to randomly guess Ahah. Also, some params are changed sometimes in ways like: user_id to userId which leaves to even more confusion. Overall amazing improvement, would be nice to have also response body examples for each too.

RuTsEST commented 4 years ago

@MelMacaluso Thanks! I agree there are still improvements to be made and we are working hard to make all of your request a reality. Keep in mind though that there are a lot of endpoints and schemas to cover and all of the manual changes made to the client will have to be merged to the new generated version of the SDK whenever we update it. So we are trying to keep those changes to a minimum.