fix: throw an error instead of ignoring it when uploading batches#207
fix: throw an error instead of ignoring it when uploading batches#207gasperzgonec wants to merge 6 commits into
Conversation
radovanjorgic
changed the title
gasperzgonec
force-pushed
the
gasperz/ASCPT-28
branch
from
27ba842 to
7a4fc6e
Compare
|
|
| const isSdkLog = message.payload?.isSdkLog ?? true; | ||
| this.logger.logFn(stringifiedArgs, level, isSdkLog); | ||
| if (typeof this.logger?.logFn === 'function') { | ||
| this.logger.logFn(stringifiedArgs, level, isSdkLog); |
There was a problem hiding this comment.
Hmm, why was this needed? Isn't logFn always a function?
| console.log( | ||
| `Uploading all repos before emitting event with event type: ${newEventType}.` | ||
| ); | ||
| const canEmit = await this.beforeEmit(newEventType); |
There was a problem hiding this comment.
canEmit calls the beforeEmit function (confusing)? And if it returns false what happens?
| parentPort?.postMessage(WorkerMessageSubject.WorkerMessageExit); | ||
| this.hasWorkerEmitted = true; | ||
| const payload = this.buildEmitPayload(newEventType, data); | ||
| const sent = await this.sendToPlatform(newEventType, payload); |
There was a problem hiding this comment.
Naming is a bit strange here IMO, can we find better name for sendToPlatform? And again, if it fails we just return, is that correct?
Maybe we could move the actual emit function here, so we have beforeEmit, actual emit logic and afterEmit in this exposed method.
| } catch (error) { | ||
| console.error('Error while uploading repos', error); | ||
| for (const repo of this.repos) { | ||
| this.artifacts = [...this.artifacts, ...repo.uploadedArtifacts]; |
There was a problem hiding this comment.
What is the point of this on error? We extend the artifacts array with repo.uploadedArtifacts? Can you explain?
| const { eventType: errorEventType } = getTimeoutErrorEventType( | ||
| this.event.payload.event_type | ||
| ); | ||
| await this.emitUploadFailure(errorEventType, error); |
There was a problem hiding this comment.
Just curious if we need separate function for this.
|
2 participants
Description
Connected Issues
Checklist
npm run testOR no tests needed.npm run test:backwards-compatibility.npm run lint.