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

Save cached config to standard directories #38

Merged
merged 17 commits into from Jul 14, 2017
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
33 changes: 33 additions & 0 deletions config-path.js
@@ -0,0 +1,33 @@
const path = require('path');
const userHome = require('user-home');

const env = process.env;
const name = 'js-v8flags';

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

Choose a reason for hiding this comment

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

Seems as though this project uses two-space indentation, not tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this project really needs an editorconfig.

return path.join(library, 'Caches', name);
}

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

// https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
function linux () {
const username = path.basename(userHome);
return path.join(env.XDG_CACHE_HOME || path.join(userHome, '.cache'), name);
}

module.exports = function (platform) {
if (platform === 'darwin') {
return macos();
}

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

return linux();
}

Choose a reason for hiding this comment

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

Missing a semicolon.

12 changes: 8 additions & 4 deletions index.js
Expand Up @@ -7,6 +7,7 @@ const fs = require('fs');
const path = require('path');
const crypto = require('crypto');
const execFile = require('child_process').execFile;
const configPath = require('./config-path.js')(process.platform);
const env = process.env;
const user = env.LOGNAME || env.USER || env.LNAME || env.USERNAME || '';
const exclusions = ['--help'];
Expand All @@ -33,10 +34,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?

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

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 +134,4 @@ module.exports = function (cb) {
};

module.exports.configfile = configfile;
module.exports.configPath = configPath;
57 changes: 51 additions & 6 deletions test.js
Expand Up @@ -16,6 +16,9 @@ function eraseHome() {
delete env.USER;
delete env.LNAME;
delete env.USERNAME;
delete env.XDG_CACHE_HOME;
delete env.LOCALAPPDATA;
delete process.platform;

Choose a reason for hiding this comment

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

This doesn’t seem very nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly needed though.

}

function setTemp(dir) {
Expand All @@ -24,11 +27,9 @@ function setTemp(dir) {

function cleanup () {
var v8flags = require('./');
var userHome = require('user-home');
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 +48,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 +63,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 +84,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('./config-path.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 Expand Up @@ -133,6 +138,46 @@ describe('v8flags', function () {
v8flags(function (err, flags) {
expect(err).to.be.null;
done();
})
});
});
});

describe('config-path', function () {
const moduleName = 'js-v8flags';

beforeEach(function() {
env.HOME = 'somehome';
cleanup();
});
afterEach(cleanup);

it('should return default linux path in other environments', function(done) {
delete require.cache[require.resolve('./config-path.js')];

Choose a reason for hiding this comment

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

Is this still necessary?

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.

Choose a reason for hiding this comment

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

It’d be best to make it so it isn’t, then. Is this somehow clearing the cache for user-home?

Copy link
Contributor Author

@Siilwyn Siilwyn Jun 7, 2017

Choose a reason for hiding this comment

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

It's been a while back but I think it does yes. I consider it bad but this pattern is used in other existing tests too. The best way to test this would be in sandboxes but that falls outside the scope of this PR.

const configPath = require('./config-path.js')('other');

expect(configPath).to.equal(
path.join(env.HOME, '.cache', moduleName)
);
done();
});

it('should return default macos path in darwin environment', function(done) {
delete require.cache[require.resolve('./config-path.js')];
const configPath = require('./config-path.js')('darwin');

expect(configPath).to.equal(
path.join(env.HOME, 'Library', 'Caches', moduleName)
);
done();
});

it('should return default windows path in win32 environment', function(done) {
delete require.cache[require.resolve('./config-path.js')];
const configPath = require('./config-path.js')('win32');

expect(configPath).to.equal(
path.join(env.HOME, 'AppData', 'Local', moduleName, 'Cache')
);
done();
});
});