node: overhaul of database and storage [INT-355]#342
node: overhaul of database and storage [INT-355]#342perf2711 wants to merge 5 commits intofeature/INT-355-storage-overhaul--sdk-corefrom
Conversation
perf2711
force-pushed
the
feature/INT-355-storage-overhaul--node
branch
from
bd1ca01 to
de49a7f
Compare
perf2711
force-pushed
the
feature/INT-355-storage-overhaul--sdk-core
branch
from
5ac6de3 to
efb9d91
Compare
perf2711
force-pushed
the
feature/INT-355-storage-overhaul--node
branch
from
de49a7f to
16c9912
Compare
perf2711
force-pushed
the
feature/INT-355-storage-overhaul--node
branch
from
16c9912 to
23c058a
Compare
| clientSetup.options.database?.enable && storage | ||
| ? { | ||
| storage, | ||
| reportRecordFactory: ReportBacktraceDatabaseRecordWithAttachmentsFactory.default(), |
There was a problem hiding this comment.
can we move it to helper or some kind of function outside the constructor to keep it clean?
There was a problem hiding this comment.
what exactly, the default() function? why?
There was a problem hiding this comment.
no, the whole object setup for the database
| clientSetup.options.database?.enable && storage | ||
| ? { | ||
| storage, | ||
| reportRecordFactory: ReportBacktraceDatabaseRecordWithAttachmentsFactory.default(), |
There was a problem hiding this comment.
no, the whole object setup for the database
|
|
||
| const report = converter.convert(JSON.parse(recordJson)); | ||
| reports.push([recordPath, report]); | ||
| reports.push([recordName, report]); |
There was a problem hiding this comment.
the model expects to store recordPath, however you're using here recordName. Type definition in L:330 needs to be updated, or we might have a bug here.
I think it would be better if we rename recordName to recordPath and keep using it in the rest of the places in the function.
| export interface BacktraceFileAttachmentFactory { | ||
| create(filePath: string, name?: string): BacktraceFileAttachment; | ||
| } |
There was a problem hiding this comment.
suggestion - I thought we keep interfaces in the top of the file. Any objection on this?
| } | ||
|
|
||
| export class NodeFsBacktraceFileAttachmentFactory implements BacktraceFileAttachmentFactory { | ||
| constructor(private readonly _fs: Pick<NodeFs, 'existsSync' | 'createReadStream'> = nodeFs) {} |
There was a problem hiding this comment.
maybe we can export a type: Pick<NodeFs, 'existsSync' | 'createReadStream'>
2 participants
Follow up to #341.
Adds required implementations for overhaul of database and storage.
Notable changes:
BacktraceStorageModuleFactoryto allow for overriding creation of storageAttachmentBacktraceDatabaseRecordand its implementationsReportBacktraceDatabaseRecordWithAttachmentsand its implementationsBacktraceFileAttachmentFactory