BaseService: Query params #1589
Conversation
Generated by 🚫 dangerJS |
# Conflicts: # services/base.js
|
@platan do you mind taking the review of this one? |
# Conflicts: # services/base.spec.js
|
It would be really great if our bus factor on this BaseService code were greater than 1. It's so essential to how Shields works! If the code is hard to understand, it's important we make it clearer… and all the more reason to address those issues before the problem becomes even harder to tackle. @platan Can I implore your review? 👐 |
paulmelnikow
changed the title
# Conflicts: # services/base.js # services/base.spec.js
|
@paulmelnikow I'm starting to read it ... |
There was a problem hiding this comment.
Changes in production code are simple and are OK. But changes in test code were difficult to understand for me. DummyService does nothing specific and you have to memorize it internals before analyzing test code. I've left some suggestions, they could improve understanding of the code.
| @@ -38,6 +38,9 @@ module.exports = class BaseService { | |||
| * - capture: Array of names for the capture groups in the regular | |||
| * expression. The handler will be passed an object containing | |||
| * the matches. | |||
| * - queryParams: Array of names for query parameters which will the service | |||
There was a problem hiding this comment.
A few lines above there is Returns an object with two fields:. Now we have 3 fields.
There was a problem hiding this comment.
Thanks, looks like this was addressed in #1582.
services/base.spec.js
Outdated
| } | ||
|
|
||
| static get category() { return 'cat'; } | ||
| static get url() { | ||
| return { | ||
| base: 'foo', | ||
| format: '([^/]+)', | ||
| capture: ['someArg'] | ||
| capture: ['someArg'], |
There was a problem hiding this comment.
Can we rename someArg to namedArgA (or namedArg1).
services/base.spec.js
Outdated
| } | ||
|
|
||
| static get category() { return 'cat'; } | ||
| static get url() { | ||
| return { | ||
| base: 'foo', | ||
| format: '([^/]+)', | ||
| capture: ['someArg'] | ||
| capture: ['someArg'], | ||
| queryParams: ['suffix'], |
There was a problem hiding this comment.
Can we rename suffix to queryParamA?
services/base.spec.js
Outdated
| message: 'Hello ' + someArg, | ||
| }; | ||
| async handle({ someArg }, { suffix }) { | ||
| return { message: `Hello ${someArg}${suffix}` }; |
There was a problem hiding this comment.
How about this?
return { message: `Hello namedParamA${namedParamA} with queryParamA${quaryParamA}` };
services/base.spec.js
Outdated
|
|
||
| const mockSendBadge = sinon.spy(); | ||
| const mockRequest = { | ||
| asPromise: sinon.spy(), | ||
| }; | ||
| await requestHandler( | ||
| /*data*/ {}, | ||
| /*queryParams*/ { suffix: '?' }, |
There was a problem hiding this comment.
What do you think about extracting the first and the second arguments to variables?
const queryParams = { suffix: '?' };
const match = '/foo/bar.svg'.match(expectedRouteRegex);
await requestHandler(
queryParams,
match,
mockSendBadge,
mockRequest
);
services/base.spec.js
Outdated
| @@ -162,7 +163,7 @@ describe('BaseService', () => { | |||
| expect(mockSendBadge).to.have.been.calledWith( | |||
| /*format*/ 'svg', | |||
There was a problem hiding this comment.
How about extracting svg to a variable?
|
Thank you so much for the review!
I'm totally open to improving this. Maybe we could think about creating a dummy service which has clearer semantics. |
This is a fairly straightforward improvement to
BaseServicewhich provides support for query parameters, in parity with the functionality in old-style servicesThis functionality is necessary to port many of the existing services, and arose while I was working on this WIP branch: master...paulmelnikow:rewrite-npm-typedefs