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.

Sign up for GitHub

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

Jump to bottom

Save cached config to standard directories #38

Merged
merged 17 commits into from Jul 14, 2017
Merged

Save cached config to standard directories #38

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9bffb32
Save cached config to standard directories
Siilwyn May 8, 2017
2b9f846
Copy env-paths module to local file
Siilwyn May 8, 2017
eaca545
Remove arrow functions
Siilwyn May 8, 2017
f11246a
Use 'user-home' module instead of native 'os.homedir'
Siilwyn May 8, 2017
dff7efb
Remove template literal
Siilwyn May 8, 2017
7e95c90
Add attribution
Siilwyn May 8, 2017
103a424
Remove unused option parameter
Siilwyn May 8, 2017
c5d1bdf
Remove env-paths from dependencies
Siilwyn May 8, 2017
ce8959a
Remove functionality other than cache path
Siilwyn May 9, 2017
5171dd9
Simplify config path retrieval
Siilwyn May 9, 2017
2830504
Rename homedir to userHome for consistency
Siilwyn May 10, 2017
9d30f74
Add unit tests for config-file
Siilwyn May 10, 2017
3ff22be
Fix space and semicolon formatting
Siilwyn May 11, 2017
819f5fa
Improve process.platform setting
Siilwyn May 11, 2017
fb42dd6
Export function and pass platform to fix Node 0.10
Siilwyn Jun 7, 2017
6730801
Add missing semicolons
Siilwyn Jun 7, 2017
fef2c76
Make charmander happy :)
Siilwyn Jun 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 63 additions & 0 deletions env-paths.js
View file
Open in desktop
@@ -0,0 +1,63 @@
// Taken from https://github.com/sindresorhus/env-paths/

const path = require('path');
const os = require('os');

const homedir = require('user-home');
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 use https://www.npmjs.com/package/homedir-polyfill

Copy link
Contributor Author

@Siilwyn Siilwyn May 9, 2017 •
edited

Choose a reason for hiding this comment

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

I'm simply using user-home because it's already a dependency.

Edit: I looked into using the homedir-polyfill but would rather stick with user-home because it has no dependencies and is used a bit more.

const tmpdir = os.tmpdir();
const env = process.env;

const macos = function (name) {
const library = path.join(homedir, 'Library');

return {
data: path.join(library, 'Application Support', name),
config: path.join(library, 'Preferences', name),
cache: path.join(library, 'Caches', name),
log: path.join(library, 'Logs', name),
temp: path.join(tmpdir, name)
};
};

const windows = function (name) {
const appData = env.LOCALAPPDATA || path.join(homedir, 'AppData', 'Local');

return {
// data/config/cache/log are invented by me as Windows isn't opinionated about this
Copy link
Member

Choose a reason for hiding this comment

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

There isn't a point in ripping this module verbatim. The usage just plucks the .cache property so the API should be reduced to that use case.

data: path.join(appData, name, 'Data'),
config: path.join(appData, name, 'Config'),
cache: path.join(appData, name, 'Cache'),
log: path.join(appData, name, 'Log'),
temp: path.join(tmpdir, name)
};
};

// https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
const linux = function (name) {
const username = path.basename(homedir);

return {
data: path.join(env.XDG_DATA_HOME || path.join(homedir, '.local', 'share'), name),
config: path.join(env.XDG_CONFIG_HOME || path.join(homedir, '.config'), name),
cache: path.join(env.XDG_CACHE_HOME || path.join(homedir, '.cache'), name),
// https://wiki.debian.org/XDGBaseDirectorySpecification#state
log: path.join(env.XDG_STATE_HOME || path.join(homedir, '.local', 'state'), name),
temp: path.join(tmpdir, username, name)
};
};

module.exports = function (name) {
if (typeof name !== 'string') {
throw new TypeError('Expected string, got ' + typeof name);
}

if (process.platform === 'darwin') {
return macos(name);
}

if (process.platform === 'win32') {
return windows(name);
}

return linux(name);
};
13 changes: 9 additions & 4 deletions index.js
View file
Open in desktop
Expand Up @@ -10,8 +10,10 @@ const execFile = require('child_process').execFile;
const env = process.env;
const user = env.LOGNAME || env.USER || env.LNAME || env.USERNAME || '';
const exclusions = ['--help'];
const envPaths = require('./env-paths.js');

const configfile = '.v8flags.'+process.versions.v8+'.'+crypto.createHash('md5').update(user).digest('hex')+'.json';
const configPath = envPaths('js-v8flags').cache;
Copy link
Member

Choose a reason for hiding this comment

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

The API should be reduced to just return the .cache property (and not generate objects with a bunch of properties). This also removes the need for attribution since it isn't even the same module anymore.


const failureMessage = [
'Unable to cache a config file for v8flags to a your home directory',
Expand All @@ -33,10 +35,12 @@ function openConfig (cb) {
return tryOpenConfig(path.join(os.tmpdir(), configfile), cb);
}

tryOpenConfig(path.join(userHome, configfile), function (err, fd) {
if (err) return tryOpenConfig(path.join(os.tmpdir(), configfile), cb);
return cb(null, fd);
});
fs.mkdir(configPath, function () {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this need to be mkdirp? By that I mean, is there a chance the multiple directory paths will need to be created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'm not sure what the proper solution is here. It should only create one directory unless the user deleted a default OS directory which would probably also break other apps...

Copy link

Choose a reason for hiding this comment

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

You can definitely count on some environments not being able to make this directory. It's a crazy world out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkellen I understand but regardless of an error or not tryOpenConfig will be called so if the directory can not be created os.tmpdir will be used. Is that enough or would using mkdirp be better in some case?

Copy link

Choose a reason for hiding this comment

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

To clarify, by "multiple" I think @phated meant nested. It doesn't seem unlikely that the user would define a nested directory in config, in which case you would either need to recreate the logic in mkdirp and loop over the directory segments to create each one, or just use mkdirp so you don't need to think about edge cases.

also, is there a reason the error isn't being handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand @phated meant nested directories. With my reply I meant that it's nested inside a default OS directory so I'm wondering if the case needs to be handled.

Regarding error handling, if for some reason we can't create the directory the next function tryOpenConfig will handle the logic of falling back to a temporary directory so that's why there is no need to handle it.

Copy link
Member

Choose a reason for hiding this comment

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

If you handle the error (and it's not EEXIST - which you wouldn't need to worry about with mkdirp) you could avoid an extra tryOpenConfig on the configPath before attempting tmpdir, right?

Copy link
Contributor Author

@Siilwyn Siilwyn May 9, 2017 •
edited

Choose a reason for hiding this comment

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

Yes that's true. Do you think using mkdirp has enough benefit: avoiding an extra tryOpenConfig in edge cases to justify including this new dependency?

Copy link
Member

Choose a reason for hiding this comment

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

Even if it isn't worth the added dependency (not sure if so), checking the error might still be worth it.

tryOpenConfig(path.join(configPath, configfile), function (err, fd) {
if (err) return tryOpenConfig(path.join(os.tmpdir(), configfile), cb);
return cb(null, fd);
});
})
}

function tryOpenConfig (configpath, cb) {
Expand Down Expand Up @@ -131,3 +135,4 @@ module.exports = function (cb) {
};

module.exports.configfile = configfile;
module.exports.configPath = configPath;
10 changes: 7 additions & 3 deletions test.js
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ function cleanup () {
if (userHome === null) userHome = __dirname;

var files = [
path.resolve(userHome, v8flags.configfile),
path.resolve(v8flags.configPath, v8flags.configfile),
path.resolve(os.tmpdir(), v8flags.configfile),
];
files.forEach(function (file) {
Expand All @@ -47,7 +47,7 @@ describe('v8flags', function () {

it('should cache and call back with the v8 flags for the running process', function (done) {
var v8flags = require('./');
var configfile = path.resolve(require('user-home'), v8flags.configfile);
var configfile = path.resolve(v8flags.configPath, v8flags.configfile);
v8flags(function (err, flags) {
expect(flags).to.be.a('array');
expect(fs.existsSync(configfile)).to.be.true;
Expand All @@ -62,7 +62,7 @@ describe('v8flags', function () {

it('should not append the file when multiple calls happen concurrently and the config file does not yet exist', function (done) {
var v8flags = require('./');
var configfile = path.resolve(require('user-home'), v8flags.configfile);
var configfile = path.resolve(v8flags.configPath, v8flags.configfile);
async.parallel([v8flags, v8flags, v8flags], function (err, result) {
v8flags(function (err2, res) {
done();
Expand All @@ -83,7 +83,11 @@ describe('v8flags', function () {
it('should fall back to writing to a temp dir if user home is unwriteable', function (done) {
eraseHome();
env.HOME = path.join(__dirname, 'does-not-exist');
// Clear require cached modules so the modified environment variable HOME is used
delete require.cache[require.resolve('./')]
delete require.cache[require.resolve('./env-paths.js')]
var v8flags = require('./');
v8flags.configPath = env.HOME;
var configfile = path.resolve(os.tmpdir(), v8flags.configfile);
v8flags(function (err, flags) {
expect(fs.existsSync(configfile)).to.be.true;
Expand Down