Add level back to tinylicious #3668
Conversation
Being able to store ops on disk, rather than fully in memory, makes tinylicious scale better as a service. The functionality used to exist but was removed due to an NPM package security warning. A prior version of level-sublevel does not take a dependency on the package with the security warning.
|
|
| private readonly db; | ||
|
|
||
| constructor(config: Provider) { | ||
| this.db = config.get("db:inMemory") ? new InMemoryDb() : new LevelDb(config.get("db:path")); |
There was a problem hiding this comment.
What will the default be? For back compat reasons with our existing usages the default should be inMemory
There was a problem hiding this comment.
Good point! Even though the config says inMemory: true, we will want to flip it have levelDb as an option. In every other case, use the inMemoryDB.
@kurtb: Can you do it?
There was a problem hiding this comment.
Default config is inMemory
There was a problem hiding this comment.
Looks good. @tylerbutler had feedback with regards to documentation and testing. I'll let him follow up.
|
@kurtb Thanks for the contribution! We're working to ensure all code introduced to the repo has documentation and some tests to help protect it from breaking. This seems especially important in this case since this code isn't exercised in any of our core scenarios today. Could you add some tests and update the Tinylicious readme with information about how to use this? |
|
If not too much trouble I'd love to see the corresponding |
|
This is simply an npm vulnerability fix and not the introduction of new functionality. This PR is a revert of #3463 which was made due to "a moderate vulnerability in npm audit, leading to an unpleasant warning for new devs trying out our hello world flow." The only actual change made is using "level-sublevel": "6.6.4" rather than the previous version. Everything else isn't new code. Latest level-sublevel itself does not have the npm audit warning but a package it depends on does. An earlier version of level-sublevel does not take a dependency on this package. |
|
@kurtb Thanks for the clarification. Given the history of this code and the state of Tinylicious testing at the moment, testing isn't needed. I would like to update the README with some info about how to activate this. Or is it on by default? What config changes do I need to make? Do I need to specify the path for the db, for example? I'll merge this in in a few hours. Feel free to push new commits here, or follow up with another PR readme update if that's easier. Thanks for being patient with us! We're doing a lot of new things at the moment and I'm sorry for the lack of communication on this PR. |
|
Thanks @tylerbutler! I also noticed @markfields added some starter tests - thanks! Once you merge this I'll follow up in the next day or so with the README updates and hopefully some tests too. |
Being able to store ops on disk, rather than fully in memory, makes tinylicious scale better as a service. The functionality used to exist but was removed due to an NPM package security warning. A prior version of level-sublevel does not take a dependency on the package with the security warning.