feat: neDB persistent datastore #48
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #48 +/- ##
==========================================
Coverage ? 61.67%
==========================================
Files ? 19
Lines ? 347
Branches ? 64
==========================================
Hits ? 214
Misses ? 133
Partials ? 0 Continue to review full report at Codecov.
|
| @@ -3,3 +3,5 @@ node_modules/ | |||
| # Prevent test coverage reports from being added | |||
| .coverage | |||
| .nyc_output | |||
|
|
|||
| /lib/data | |||
There was a problem hiding this comment.
Are we storing the data in the project directory itself?
There was a problem hiding this comment.
Yeah right now we are. I'll move this to a config file.
| await this.queueProcessor(item); | ||
| }, Promise.resolve()); | ||
|
|
||
| return this.processQueue; |
There was a problem hiding this comment.
Shouldn't this be a function call?
dashboard/lib/controllers/run.js
Outdated
| }, | ||
|
|
||
| addEvent: async (data) => { | ||
| const batch = new BatchQueue(api.saveEvent); |
There was a problem hiding this comment.
We are creating a new queue every time this function is called. Is this expected? 🤔
There was a problem hiding this comment.
Yes, my bad. Should I initialize separate queues in this file and use them?
There was a problem hiding this comment.
Yes. The queue should be initialised once. Either when the file is imported or when the first call is made.
Also, do note that:
- If multiple actions depend on each other, they should use a single queue.
- If actions are independent but depend on multiple calls of itself, it should use a single queue.
- If actions are not dependent on multiple calls of itself either, it doesn't need a queue.
| const Table = require('./table'); | ||
| const connect = require('camo').connect; | ||
| const paths = require('env-paths')('newman-dashboard'); | ||
| const uri = `nedb://${paths.data}`; |
There was a problem hiding this comment.
Can we create a separate PR for config files?
| @@ -1,4 +1,4 @@ | |||
| import store from '../state/stores/runStore'; | |||
| import store from "../state/stores/runStore"; | |||
There was a problem hiding this comment.
Why this change? Should we add a lint rule to always use single quotes? 🤔
There was a problem hiding this comment.
Yes, this is my VSCode acting up. There is a bug with the ESLint extension. 🙄
| return cache[tableName]; | ||
| }, | ||
| // cleanup function to terminate db connection | ||
| return () => process.exit(0); |
There was a problem hiding this comment.
The cleanup function would terminate the whole process! Shouldn't we only close the connection?
| if (index == currentQueue.length - 1) { | ||
| this.processing = false | ||
| // at last, mark processing as complete so that new batch is saved | ||
| if (index == currentQueue.length - 1) { |
There was a problem hiding this comment.
Nitpick: Use ===. Should enforce in eslint.
There was a problem hiding this comment.
Oops, will fix this asap.
|
|
||
| // at last, mark processing as complete so that new batch is saved | ||
| if (index == currentQueue.length - 1) { | ||
| this.processing = false; |
There was a problem hiding this comment.
Let it be handled on line 15. Just call this method after the reduce. It'll take care of processing the next batch. It'll keep our loop clean.
There was a problem hiding this comment.
Oh okay, I also tried that initially but thought it would lead to too many calls. Would this be a decent tradeoff?
There was a problem hiding this comment.
It would be just 1 extra call, no? The benefit would be that changing the flag would be kept at a single place making it easier to read and track.
| addEvent: async (data) => { | ||
| if (!data.id) throw new TypeError('Invalid id'); | ||
| const run = await api.findOne(data.id); | ||
| saveEvent: async (data) => { |
There was a problem hiding this comment.
Let's make these methods (saveEvent and saveStats) private?
There was a problem hiding this comment.
These methods are defined in an object and not a class, can they be private as well? 🤔
There was a problem hiding this comment.
Yep, just don't include them in the object. It's not using this so it doesn't need to be in the object. Just define it before the object and it should be good.
Sidenote: If it did use this, you can rename it to _saveEvent to convey the intention that it is a private method.
There was a problem hiding this comment.
On it @coditva, just busy with my endsems this week. Will get this done asap
Add neDB to create a persistent file based data store for storing runs.