fix: use named parameters for postInParallelWithAutomaticChunking #1048
Conversation
I don't have full insight into why this needs to happen but this is reported as a bug by SLB, and I need code review on this. Please take a look.
trygveu
requested review from
BugGambit,
andersfylling and
a team
as code owners
|
Can you just name all the parameters so we don't have to rely on ordering. |
andersfylling
changed the title
packages/core/src/baseResourceApi.ts
Outdated
| return this.postInParallelWithAutomaticChunking({ | ||
| queryParams: params, | ||
| path: path, | ||
| items: items, | ||
| }); |
There was a problem hiding this comment.
@saagar-cognite The change looks good to me but I'm not really sure because this could also be a breaking change. Because if callRetrieveEndpoint actually meant to use params and not queryParam then this would break the implementation.
I saw that you were the last one to work on this so mentioning you.
There was a problem hiding this comment.
I think the "proper" fix would be to rename the params prop to something more meaningful like payload, body or whatever it represents.
There was a problem hiding this comment.
I suggest to send both params & queryParams as both are supported in postInParallelWithAutomaticChunking.
It spread ...params in data while sending to post call.
This option would be safest if this is a time intensive change.
There was a problem hiding this comment.
Also, I could not understand from ticket, what exactly is user trying to send which is not working.
Only thing I could infer is that they actually want to send queryParams but are not able to.
There was a problem hiding this comment.
@saagar-cognite could you ask directly in the issue? support might be able to help you get the answer from customer, i dont think they would go inside the PR to answer.
There was a problem hiding this comment.
@saagar-cognite I update the PR. Looks like the user had the same idea as us. So if they are only using the function from the core package then it should be alright
@andersfylling Would love to do that, but that would be a breaking change, don't know if we want to go that way |
There was a mismatch in ordering of parameters that has caused a blocking bug for SLB. They propose a 1-line change, naming the parameters to resolve this issue. PTAL.
Ref: https://cognitedata.atlassian.net/browse/CDF-20687