PDE-476: Handling omitEmptyParams #121
Conversation
Includes tests and has been tested in a CLI app in production with a modified core. Requires a schema PR.
| // Take params off of req.params and append to url - "?a=1&b=2"". | ||
| // This middleware should run *after* custom middlewares, because | ||
| // custom middlewares might add params. | ||
| const addQueryParams = req => { | ||
| if (Object.keys(req.params || {}).length) { | ||
| const splitter = req.url.indexOf('?') === -1 ? '?' : '&'; | ||
| req.url += splitter + querystring.stringify(req.params); | ||
|
|
||
| if (req.omitEmptyParams === true) { |
There was a problem hiding this comment.
Can this just be if (req.omitEmptyParams)?
There was a problem hiding this comment.
Looks great! Love the tests. Had some thoughts about url parsing but nothing showstopping.
One last thing- do we have tests that cover the splitter code? It seems like all of the tests (here and previously) call /get, but none with /get?a=1. I may be missing it, but I don’t think anyging uses & as the splitter.
| @@ -2,13 +2,34 @@ | |||
|
|
|||
| const querystring = require('querystring'); | |||
|
|
|||
| // Mutates the object (it'll be deleted later anyway) | |||
| const removeEmptyParams = params => | |||
There was a problem hiding this comment.
I don’t love that this mutates, but given that it’s local and commented, it’s probably fine
There was a problem hiding this comment.
Same here, but since the variable was being mutated/removed before, I went with the less code option here.
| const stringifiedParams = querystring.stringify(req.params); | ||
|
|
||
| if (stringifiedParams) { | ||
| req.url += `${splitter}${stringifiedParams}`; |
There was a problem hiding this comment.
I think this is fine as is, but I think we’d be better off parsing the url with the stdlib, pulling out the query, merging with new params, and adding the whole thing rather than guessing the splitter and adding it that way.
Again, it’s fine, but using established tools feels a little more above the board.
There was a problem hiding this comment.
Agreed. I just didn't want to change too much here, honestly.
We only test the splitter now by not adding ? (it was in the case the params existed but where then removed).
Includes tests and has been tested in a CLI app in production with a modified core.
Requires a schema PR: zapier/zapier-platform-schema#57
Fixes PDE-476.