Implementing FCM sendAll() API #453
Conversation
| constructor(resp: LowLevelResponse) { | ||
| this.status = resp.status; | ||
| this.headers = resp.headers; | ||
| this.multipart = resp.multipart; |
There was a problem hiding this comment.
should you validate it is an actually multipart response?
There was a problem hiding this comment.
This class is not exported, and we control the only place where it gets instantiated. So it should be fine.
| respStream.on('data', (chunk: Buffer) => { | ||
| responseBuffer.push(chunk); | ||
| }); | ||
| const boundary = getMultipartBoundary(res.headers); |
There was a problem hiding this comment.
sendRequest has become too large, is there a way we can split it to be more manageable? or create smaller functions to handle different parts of the logic?
There was a problem hiding this comment.
It's not easy due to the use of new Promise() but I think we can do some improvements. However, I'd rather do that as a separate PR.
| const headerLines: string[] = responseText.substring(0, endOfHeaderPos).split('\r\n'); | ||
|
|
||
| const statusLine: string = headerLines[0]; | ||
| const status: string = statusLine.trim().split(/\s/)[1]; |
There was a problem hiding this comment.
Validate that the array has >=2 elements and before that headerLines has at least 1 element.
There was a problem hiding this comment.
slice(1) returns an empty array when length is less than 2. So it's safe to iterate over it without a length check. Similarly, headerLines is the result of a split() call, which always returns an array with at least 1 element.
There was a problem hiding this comment.
I only see that you are using split here and not slice:
statusLine.trim().split(/\s/)[1]
There was a problem hiding this comment.
We use split and slice for headerLines. For statusLine I added a check to cover the case of a malformed header. Although chances of it happening should be negligibly small.
|
Thanks @bojeil-google. I've responded to your comments. PTAL. |
| const headerLines: string[] = responseText.substring(0, endOfHeaderPos).split('\r\n'); | ||
|
|
||
| const statusLine: string = headerLines[0]; | ||
| const status: string = statusLine.trim().split(/\s/)[1]; |
There was a problem hiding this comment.
I only see that you are using split here and not slice:
statusLine.trim().split(/\s/)[1]
| }); | ||
|
|
||
| function parseHttpRequest(text: string | Buffer): any { | ||
| const httpMessageParser = require('http-message-parser'); |
There was a problem hiding this comment.
Suggestion: I think we should minimize depending on low usage npm modules. In this case, this has ~2k weekly downloads. We have been bitten by some dependency on an npm module that was abandoned in the past.
There was a problem hiding this comment.
We only use this in a couple of test cases. I can try to replace it with something else in a future PR.
| ).should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential'); | ||
| }); | ||
|
|
||
| function checkSendResponseSuccess(response: SendResponse, messageId: string) { |
There was a problem hiding this comment.
nit: define helper functions on top of file.
There was a problem hiding this comment.
Done. Moved to the top of the enclosing block. These helpers are scoped to a specific test case.
|
Thanks @bojeil-google. I've addressed most of your comments. |
* Implementing sendMulticast() API * Added integration test * Fixed a comment * Readability improvement in the copy operation
…de into hkj-fcm-batch
Implementing the following new API:
This implements a part of go/fcm-multicast. I've also implemented the following refactorings in order to keep the size of the
messaging.tsmodule in check:messaging-types.tsmodule.messaging-errors.tsmodule.There's more room for refactoring here, but I would rather do that in separate PRs.