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

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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions config-path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');
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();
};
12 changes: 8 additions & 4 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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?

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
Loading

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;
56 changes: 50 additions & 6 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ function eraseHome() {
delete env.USER;
delete env.LNAME;
delete env.USERNAME;
delete env.XDG_CACHE_HOME;
delete env.LOCALAPPDATA;
}

function setTemp(dir) {
Expand All @@ -24,11 +26,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 +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('./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 +137,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')];
Copy link

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.

Copy link

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 •
edited
Loading

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();
});
});