feat: added support for path parameters#16
Conversation
vasan-agrostar
left a comment
vasan-agrostar
left a comment
There was a problem hiding this comment.
Great job! I was expecting you to miss out the schema change. You have done that too. Wonderful. But there are some things we need to address. I have left comments.
schemas/zzapi-bundle.schema.json
Outdated
| "description": "path parameters", | ||
| "anyOf": [ | ||
| { | ||
| "type": "array", |
There was a problem hiding this comment.
How does an array apply to pathParams? I don't think we should allow this.
There was a problem hiding this comment.
"pathParamObject": {
"type": "object",
"description": "A set of path parameters described as key/value pairs",
"properties": {
"": { "$ref": "#/$defs/scalar" }
}
}
tried with this type but user can still pass array of values.
| allData.httpRequest.baseUrl, | ||
| allData.httpRequest.url, | ||
| getParamsForUrl(allData.httpRequest.params, allData.options.rawParams), | ||
| ); |
There was a problem hiding this comment.
I would rather pass the pathParams here and get the getURL() function to incorporate it.
src/executeRequest.ts
Outdated
| getParamsForUrl(allData.httpRequest.params, allData.options.rawParams), | ||
| ); | ||
|
|
||
| if(allData.httpRequest.pathParams){ |
There was a problem hiding this comment.
Ideally, we should check if the path has /:\w+/ and call substitute. What if the user has forgotten to add pathParams, but has /:userid/ in the path? We got to tell the user that a value is expected, no?
There was a problem hiding this comment.
Right, the condition is useless, this will run every time. Updated to check this with regex.
| if (param) { | ||
| return encodeURIComponent(param.value); | ||
| } | ||
| throw new Error(`Missing value for parameter: ${key}`); |
There was a problem hiding this comment.
Not sure if a throw translate to a user-friendly readable message in the output window. Have you tested this path?
There was a problem hiding this comment.
Yes it shows an error message about the missing parameter.
src/executeRequest.ts
Outdated
| return baseUrl + path.replace(/:(\w+)/g, (_, key) => { | ||
| const param = params.find(p => p.name === key); | ||
| if (param) { | ||
| return encodeURIComponent(param.value); |
There was a problem hiding this comment.
Not sure if we should encodeURI component. Things like "/" in the path parameter are very problematic, even if you encode them. Path parameters are not intended to have complex strings, just IDs, typically numbers. And sometimes slugs, which too, don't have non-word non-number characters.
|
Hey @vasan-agrostar, I've made changes addressing your comments. Let me know if this looks good. |
2 participants
example.zzb