Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added multi-directory support #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions lib/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,46 @@ const mkdirp = require('mkdirp');
const path = require('path');
const utils = require('./utils');

/**
* @summary Set data path
* @function
* @public
*
* @description
* Pass the directory you want to call the functions from.
* If no directory is passed, returns to default 'storage'
*
* @param {String} directory - directory
*
* @example
* const storage = require('electron-json-storage');
*
* storage.setDataPath('foo');
*/
exports.setDataPath = function(dir = ""){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're exposing both functions from utils, what do you think about simply moving their logic here?

utils.setUserDataPath(dir);
};

/**
* @summary Get data path
* @function
* @public
*
* @description
* Returns the current directory being used.
*
* @param {String} directory - directory
*
* @example
* const storage = require('electron-json-storage');
*
* let dataPath = storage.getDataPath();
* console.log(dataPath);
*/
exports.getDataPath = function(){
return utils.getUserDataPath();
};

/**
* @summary Read user data
* @function
Expand Down Expand Up @@ -121,7 +161,7 @@ exports.getMany = function(keys, callback) {
if (error) {
return callback(error);
}

// console.log(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be deleted.

return callback(null, _.set(reducer, key, data));
});
}, callback);
Expand Down Expand Up @@ -255,9 +295,12 @@ exports.keys = function(callback) {
},
fs.readdir,
function(keys, callback) {
callback(null, _.map(keys, function(key) {
return path.basename(key, '.json');
}));
callback(null, keys.filter(function(key){
return path.extname(key) !== "";
}).map(function(key) {
return path.basename(key, '.json');
}
));
}
], callback);
};
Expand Down
39 changes: 37 additions & 2 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment its too redundant, and not contributing anything of value here.

const defaultBasePath = 'storage';
let currentDirectory = '';

/**
* @summary Parse directory to avoid parent directories
* @function
* @private
*
* @returns {Strings} parsed user data path without parent directories
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be String.

*/
function getDirectory(directory){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should inline 'storage' here, or create a constant within the scope of exports.getUserDataPath, given that is not used elsewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a consequence, path.join(app.getPath('userData'), 'storage') should be the initial value of currentDirectory.

const directoryArray = directory.split(path.posix.sep);
let newDirectory = "";
for(var dir of directoryArray){
if(dir !== ".."){
newDirectory += dir + "/";
}
}
return newDirectory;
}

/**
* @summary Set default user data directory path
* @function
* @public
*
* @example
* utils.getUserDataPath('newDirectory');
*/
exports.setUserDataPath = function(directory) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

currentDirectory = getDirectory(directory);
};

/**
* @summary Get user data directory path
* @summary Get default user data directory path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, its not really the default one, given that the result depends on the directory the user set. Right?

* @function
* @public
*
* @returns {Strings} user data path
*
* @example
* let userDataPath = utils.getUserDataPath();
* console.log(userDataPath);
*/
exports.getUserDataPath = function() {
return path.join(app.getPath('userData'), 'storage');
return path.join(app.getPath('userData'), defaultBasePath, currentDirectory);
};

/**
Expand Down