added multi-directory support #51
Conversation
An initial start to add directory support, looking to get help/advice from community to make code more robust. Added capability to prefix key with directory for get, set, has, remove (i.e. "testfolder/foo" will get, set, has, or remove foo under the testfolder directory). Also extended functions getAll, keys, clear* to allow optional directory parameter (i.e. "testfolder" will getAll, keys or clear* under the testfolder directory. getMany is tricky and needs some work. *Currently clear removes the directory passed to function. **All current test cases pass with exception of utils test function since directory is different
|
Hi @kfardanesh , Thanks a lot for the PR, and sorry for not having time to look at this during the weekend. I'll try to do so on Monday :) |
|
Let's add some functions to set and get the default settings location, for example: Having to specify the directory each time looks like unnecessary extra overhead. |
|
I reverted my changes to your original functions and added the get and set functions for the current working directory. All original test cases still work. A slight annoyance with most recent commit is that any new directories created under the base 'storage' folder will persist unless deleted manually. |
| * let userDataPath = utils.getUserDataPath(); | ||
| * console.log(userDataPath); | ||
| */ | ||
| exports.getUserDataPath = function() { | ||
| return path.join(app.getPath('userData'), 'storage'); |
There was a problem hiding this comment.
I think we should inline 'storage' here, or create a constant within the scope of exports.getUserDataPath, given that is not used elsewhere.
| /** | ||
| * @summary Get user data directory path | ||
| * @summary Get default user data directory path |
There was a problem hiding this comment.
Well, its not really the default one, given that the result depends on the directory the user set. Right?
| * @function | ||
| * @private | ||
| * | ||
| * @returns {Strings} parsed user data path without parent directories |
| @@ -29,18 +29,53 @@ const path = require('path'); | |||
| const electron = require('electron'); | |||
| const app = electron.app || electron.remote.app; | |||
|
|
|||
| // define the defaultDataPath and current directory | |||
There was a problem hiding this comment.
I think the comment its too redundant, and not contributing anything of value here.
| * | ||
| * @returns {Strings} parsed user data path without parent directories | ||
| */ | ||
| function getDirectory(directory){ |
There was a problem hiding this comment.
I like the security thinking here, however I don't think we have any good reason to restrict the settings path to be inside a certain directory. A good use case of this feature is that people can store settings whenever they want, so I guess we can leave out this normalization routine.
There was a problem hiding this comment.
As a consequence, path.join(app.getPath('userData'), 'storage') should be the initial value of currentDirectory.
| * @example | ||
| * utils.getUserDataPath('newDirectory'); | ||
| */ | ||
| exports.setUserDataPath = function(directory) { |
There was a problem hiding this comment.
Based on https://github.com/jviotti/electron-json-storage/pull/51/files#r105062802, I think we should do a sanity check here to ensure that directory is an absolute path, using path.isAbsolute(), and throw an error otherwise.
| @@ -121,7 +161,7 @@ exports.getMany = function(keys, callback) { | |||
| if (error) { | |||
| return callback(error); | |||
| } | |||
|
|
|||
| // console.log(data) | |||
There was a problem hiding this comment.
This should probably be deleted.
| * | ||
| * storage.setDataPath('foo'); | ||
| */ | ||
| exports.setDataPath = function(dir = ""){ |
There was a problem hiding this comment.
Now that we're exposing both functions from utils, what do you think about simply moving their logic here?
|
Any update on this feature? Either directory support in the keys or changing the user path. I would be interested in this functionality. |
|
@stromboul I don't think the author is still working on it. You're free to take over using the suggestions in this PR! |
An initial start to add multi-directory support while keeping existing functionality. Looking to get help/advice from community to make code more robust and describe what functionality they would like to see. I understand that this involves a lot more edge cases and effort to maintain than the single directory so also looking to gauge interest since this is one of the more popular repos for json storage on electron.
Added capability to prefix key with directory for get(), set(), has(), remove() (i.e. "testfolder/foo" will get, set, has, or remove foo under the testfolder directory).
Also extended functions getAll(), keys(), clear()* to allow optional directory parameter (i.e. "testfolder" will getAll, keys or clear* under the testfolder directory)
getMany() is tricky and needs some work to determine functionality with multi-directories.
*Currently clear() removes the directory passed to function. Can be changed.
**All current test cases pass with exception of utils test function which I commented out for time being.
***Can work on adding test cases if there is enough interest