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

Conversation

Projects
None yet
5 participants
@Siilwyn
Copy link
Contributor

Siilwyn commented May 8, 2017

Instead of placing the configuration file in the home directory it should be placed in the appropriate directory per OS.

Breaking change: resolves issue #37

Siilwyn added some commits May 8, 2017

Save cached config to standard directories
Instead of placing the configuration file in the home directory it should be placed in the [appropriate directory](https://wiki.archlinux.org/index.php/XDG_Base_Directory_support) per OS.

Breaking change: resolves issue #37

@Siilwyn Siilwyn changed the title WIP: Save cached config to standard directories Save cached config to standard directories May 8, 2017

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented May 8, 2017

@phated & @tkellen ready for review!

@phated
Copy link
Member

phated left a comment

Thanks. This can be reduced by a lot though. There also needs to be tests over the new .js file.

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

const homedir = require('user-home');

This comment has been minimized.

@phated

This comment has been minimized.

@Siilwyn

Siilwyn May 9, 2017

Contributor

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 appData = env.LOCALAPPDATA || path.join(homedir, 'AppData', 'Local');

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

This comment has been minimized.

@phated

phated May 8, 2017

Member

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.

index.js Outdated

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

This comment has been minimized.

@phated

phated May 8, 2017

Member

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.

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

This comment has been minimized.

@phated

phated May 8, 2017

Member

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

This comment has been minimized.

@Siilwyn

Siilwyn May 9, 2017

Contributor

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...

This comment has been minimized.

@tkellen

tkellen May 9, 2017

Member

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

This comment has been minimized.

@Siilwyn

Siilwyn May 9, 2017

Contributor

@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?

This comment has been minimized.

@jonschlinkert

jonschlinkert May 9, 2017

Member

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?

This comment has been minimized.

@Siilwyn

Siilwyn May 9, 2017

Contributor

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.

This comment has been minimized.

@phated

phated May 9, 2017

Member

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?

This comment has been minimized.

@Siilwyn

Siilwyn May 9, 2017

Contributor

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?

This comment has been minimized.

@phated

phated May 10, 2017

Member

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


const env = process.env;

const macos = function (name) {

This comment has been minimized.

@phated

phated May 9, 2017

Member

Prefer named functions over function assignments

This comment has been minimized.

@phated

phated May 9, 2017

Member

Instead of taking a name, can just have 'js-v8flags' assigned as a variable at the top of file.

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

const windows = function (name) {

This comment has been minimized.

@phated

phated May 9, 2017

Member

Prefer named functions over function assignments

This comment has been minimized.

@phated

phated May 9, 2017

Member

Instead of taking a name, can just have 'js-v8flags' assigned as a variable at the top of file.

};

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

This comment has been minimized.

@phated

phated May 9, 2017

Member

Prefer named functions over function assignments

This comment has been minimized.

@phated

phated May 9, 2017

Member

Instead of taking a name, can just have 'js-v8flags' assigned as a variable at the top of file.

return path.join(env.XDG_CACHE_HOME || path.join(homedir, '.cache'), name);
};

module.exports = function (name) {

This comment has been minimized.

@phated

phated May 9, 2017

Member

Instead of calling this as a function, it should probably just export the proper path based on platform

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

This comment has been minimized.

@phated

phated May 9, 2017

Member

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?

module.exports = windows();
}

module.exports = linux();

This comment has been minimized.

@phated

phated May 9, 2017

Member

This will always overwrite the value with the result of linux(). I believe this outlines why this needs extra unit tests added.

This comment has been minimized.

@Siilwyn

Siilwyn May 10, 2017

Contributor

Oh whoops my bad, it's because I quickly removed it from the function late in the evening ^^.
What kind of unit tests would you add, hardcode the expected path in the test?

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented May 10, 2017

For some reason the CI test on Node 0.10 fails at one unit test and I don't understand why. Any help would be appreciated! :)

if (process.platform === 'darwin') {
module.exports = macos();
} else if (process.platform === 'win32') {
module.exports = windows();

This comment has been minimized.

@phated

phated May 10, 2017

Member

extra space after =

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

This comment has been minimized.

@phated

phated May 10, 2017

Member

no semi-colon after named functions

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

This comment has been minimized.

@phated

phated May 10, 2017

Member

no semi-colon after named functions

function linux () {
const username = path.basename(userHome);
return path.join(env.XDG_CACHE_HOME || path.join(userHome, '.cache'), name);
};

This comment has been minimized.

@phated

phated May 10, 2017

Member

no semi-colon after named functions

test.js Outdated
afterEach(cleanup);

it('should return default linux path in other environments', function(done) {
Object.defineProperty(process, 'platform', {value: 'other'});

This comment has been minimized.

@phated

phated May 10, 2017

Member

This needs to be reset/cleaned-up

test.js Outdated
});

it('should return default macos path in darwin environment', function(done) {
Object.defineProperty(process, 'platform', {value: 'darwin'});

This comment has been minimized.

@phated

phated May 10, 2017

Member

This needs to be reset/cleaned-up

test.js Outdated
});

it('should return default windows path in win32 environment', function(done) {
Object.defineProperty(process, 'platform', {value: 'win32'});

This comment has been minimized.

@phated

phated May 10, 2017

Member

This needs to be reset/cleaned-up

return path.join(env.XDG_CACHE_HOME || path.join(userHome, '.cache'), name);
}

if (process.platform === 'darwin') {

This comment has been minimized.

@tkellen

tkellen May 12, 2017

Member

While I doubt it matters for this module, this approach for dynamic exporting makes it impossible to statically analyze this. Can you update this to export a single function that takes process.platform as an argument and returns the correct result accordingly?

This comment has been minimized.

@Siilwyn

Siilwyn May 12, 2017

Contributor

You realize that it was initially exporting a function but phated told me to change it?

This comment has been minimized.

@tkellen

tkellen May 12, 2017

Member

My apologies, I did not--I have not been watching as closely as he has. Mind sharing what you think the most appropriate approach is here?

This comment has been minimized.

@Siilwyn

Siilwyn May 12, 2017

Contributor

No problem, I'm not sure as both ways of their pros and cons. Think as it is now it's very specific to this package so I would leave it as it is. When this package drops support for < Node 4 ideally we would swap it with the env-paths package, by then the 'new ES import' will be in place too so it has more significance to be able to statically analyze it.
🤔

This comment has been minimized.

@Siilwyn

Siilwyn May 12, 2017

Contributor

Speaking of Node 0.10, do you have any idea why it's failing in CI? Really need some help to get that fixed. :(

This comment has been minimized.

@Siilwyn

Siilwyn May 13, 2017

Contributor

@phated yes I already tried debugging locally but can't figure it out. Node 0.10 is acting weird, test doesn't find the user-home module when process.platform is win32. Just changing process.platform is enough to make it work. :/
All newer Node versions work fine.

This comment has been minimized.

@Siilwyn

Siilwyn May 20, 2017

Contributor

Alright after giving this more debugging time I'm still stuck. What do you think of reversing the earlier change and returning a function instead passing it the platform instead of using a global?
Or dropping 0.10 support but you probably have a reason not to ...?

This comment has been minimized.

@phated

phated May 22, 2017

Member

We aren't dropping 0.10 support because it is still the latest node version shipped in official channels on some linux.

I don't like returning the function; we should really just figure out what is going on here.

This comment has been minimized.

@charmander

charmander May 23, 2017

It’s not returning the function; it’s exporting it.

module.exports = function (process) {
    if (process.platform === …) {
        ⋮
    }
};

would make it possible to pass an object in in tests without changing process itself, which is what causes the breakage on 0.10, and additionally clean up the require cache hack.

This comment has been minimized.

@Siilwyn

Siilwyn May 23, 2017

Contributor

Oops, bad wording from my end. That's what I meant. @phated thoughts?

Siilwyn added some commits Jun 7, 2017

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented Jun 7, 2017

It's working with all versions now.

}

return linux();
}

This comment has been minimized.

@charmander

charmander Jun 7, 2017

Missing a semicolon.

const name = 'js-v8flags';

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

This comment has been minimized.

@charmander

charmander Jun 7, 2017

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

This comment has been minimized.

@Siilwyn

Siilwyn Jun 7, 2017

Contributor

Correct, this project really needs an editorconfig.

test.js Outdated
@@ -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;

This comment has been minimized.

@charmander

charmander Jun 7, 2017

This doesn’t seem very nice.

This comment has been minimized.

@Siilwyn

Siilwyn Jun 7, 2017

Contributor

It's mostly needed though.

afterEach(cleanup);

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

This comment has been minimized.

@charmander

charmander Jun 7, 2017

Is this still necessary?

This comment has been minimized.

@Siilwyn

Siilwyn Jun 7, 2017

Contributor

Yes.

This comment has been minimized.

@charmander

charmander Jun 7, 2017

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

This comment has been minimized.

@Siilwyn

Siilwyn Jun 7, 2017

Contributor

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.

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented Jun 19, 2017

@tkellen & @phated thoughts?

@phated

This comment has been minimized.

Copy link
Member

phated commented Jun 20, 2017

@Siilwyn I needed to put this on the backburner to push through the last couple of gulp4 blockers. I'll do a more in-depth review of everything once that is shipped.

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented Jun 21, 2017

Okay nice! I already love using Gulp 4 in the alpha.

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented Jun 27, 2017

@tkellen do you have some time to review in advance?

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented Jul 13, 2017

@tkellen could you review the PR? :)

@tkellen

This comment has been minimized.

Copy link
Member

tkellen commented Jul 14, 2017

Apologies for the delay @Siilwyn. I've just added you as a collaborator on this repo / author on npm. Thank you for this work!

I'm actually leaving for a much needed vacation this morning and will be offline for the next week. If you'd like to land and publish this, please feel free to do so as long as you keep a close eye on the issue tracker and fix any issues that crop up as a result.

@Siilwyn

This comment has been minimized.

Copy link
Contributor

Siilwyn commented Jul 14, 2017

Thank you for your reply and the given trust @tkellen! I'll let this sit for a bit more today so there's some room for further replies.

Enjoy your vacation, going offline for a while is always refreshing!

@Siilwyn Siilwyn merged commit a1845cc into gulpjs:master Jul 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Siilwyn Siilwyn deleted the Siilwyn:cache-to-standard-directory branch Jul 14, 2017

elhigu added a commit to tgriesser/knex that referenced this pull request Oct 25, 2017

Update v8flags to version 3.0.0 (#2288)
The v8flags dependency got updated so that it follows the OS cache location conventions. See [the PR](gulpjs/v8flags#38) for more information.

@Siilwyn Siilwyn referenced this pull request Oct 9, 2018

Open

XDG compliance #2334

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment