Save cached config to standard directories #38

Siilwyn · 2017-05-08T16:18:08Z

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

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 · 2017-05-08T17:43:39Z

@phated & @tkellen ready for review!

phated

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

phated · 2017-05-08T22:21:15Z

env-paths.js

+const path = require('path');
+const os = require('os');
+
+const homedir = require('user-home');


This should use https://www.npmjs.com/package/homedir-polyfill

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.

phated · 2017-05-08T22:24:52Z

env-paths.js

+	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


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.

phated · 2017-05-08T22:26:03Z

index.js


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


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.

phated · 2017-05-08T22:27:03Z

index.js

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


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

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

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

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

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?

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.

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?

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?

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

phated · 2017-05-09T16:35:38Z

cache-paths.js

+
+const env = process.env;
+
+const macos = function (name) {


Prefer named functions over function assignments

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

phated · 2017-05-09T16:35:41Z

cache-paths.js

+	return path.join(library, 'Caches', name);
+};
+
+const windows = function (name) {


Prefer named functions over function assignments

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

phated · 2017-05-09T16:35:49Z

cache-paths.js

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


Prefer named functions over function assignments

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

phated · 2017-05-09T16:39:20Z

cache-paths.js

+	return path.join(env.XDG_CACHE_HOME || path.join(homedir, '.cache'), name);
+};
+
+module.exports = function (name) {


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

phated · 2017-05-09T16:41:55Z

index.js

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


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?

phated · 2017-05-09T21:48:14Z

config-path.js

+	module.exports =  windows();
+}
+
+module.exports = linux();


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

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 · 2017-05-10T15:16:34Z

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! :)

phated · 2017-05-10T18:56:23Z

config-path.js

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


extra space after =

phated · 2017-05-10T18:56:45Z

config-path.js

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


no semi-colon after named functions

phated · 2017-05-10T18:56:48Z

config-path.js

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


no semi-colon after named functions

phated · 2017-05-10T18:56:53Z

config-path.js

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


no semi-colon after named functions

phated · 2017-05-10T18:58:45Z

test.js

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


This needs to be reset/cleaned-up

phated · 2017-05-10T18:58:50Z

test.js

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


This needs to be reset/cleaned-up

phated · 2017-05-10T18:58:53Z

test.js

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


This needs to be reset/cleaned-up

tkellen · 2017-05-12T01:07:37Z

config-path.js

+	return path.join(env.XDG_CACHE_HOME || path.join(userHome, '.cache'), name);
+}
+
+if (process.platform === 'darwin') {


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?

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

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?

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

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

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

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

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.

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.

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

Siilwyn · 2017-06-07T09:43:06Z

It's working with all versions now.

charmander · 2017-06-07T10:00:13Z

config-path.js

+	}
+
+	return linux();
+}


Missing a semicolon.

charmander · 2017-06-07T10:01:35Z

config-path.js

+const name = 'js-v8flags';
+
+function macos () {
+	const library = path.join(userHome, 'Library');


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

Correct, this project really needs an editorconfig.

charmander · 2017-06-07T10:03:52Z

test.js

@@ -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 doesn’t seem very nice.

It's mostly needed though.

charmander · 2017-06-07T10:06:53Z

test.js

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


Is this still necessary?

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

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 · 2017-06-19T08:29:36Z

@tkellen & @phated thoughts?

phated · 2017-06-20T22:44:38Z

@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 · 2017-06-21T07:09:32Z

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

Siilwyn · 2017-06-27T11:02:45Z

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

Siilwyn · 2017-07-13T21:21:55Z

@tkellen could you review the PR? :)

tkellen · 2017-07-14T11:57:44Z

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 · 2017-07-14T12:06:14Z

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! ⛱️

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

Siilwyn added 7 commits May 8, 2017 18:15

Save cached config to standard directories

9bffb32

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

Copy env-paths module to local file

2b9f846

Remove arrow functions

eaca545

Use 'user-home' module instead of native 'os.homedir'

f11246a

Remove template literal

dff7efb

Add attribution

7e95c90

Remove unused option parameter

103a424

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

Remove env-paths from dependencies

c5d1bdf

phated requested changes May 8, 2017

View reviewed changes

Remove functionality other than cache path

ce8959a

phated requested changes May 9, 2017

View reviewed changes

phated reviewed May 9, 2017

View reviewed changes

Siilwyn added 3 commits May 10, 2017 09:51

Simplify config path retrieval

5171dd9

Rename homedir to userHome for consistency

2830504

Add unit tests for config-file

9d30f74

phated requested changes May 10, 2017

View reviewed changes

Siilwyn added 2 commits May 11, 2017 20:42

Fix space and semicolon formatting

3ff22be

Improve process.platform setting

819f5fa

tkellen reviewed May 12, 2017

View reviewed changes

Siilwyn added 2 commits June 7, 2017 11:36

Export function and pass platform to fix Node 0.10

fb42dd6

Add missing semicolons

6730801

charmander suggested changes Jun 7, 2017

View reviewed changes

Make charmander happy :)

fef2c76

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

Siilwyn deleted the cache-to-standard-directory branch July 14, 2017 19:03

Siilwyn mentioned this pull request Jul 19, 2017

Update v8flags to version 3.0.0 babel/babel#5975

Merged

Siilwyn mentioned this pull request Oct 24, 2017

Update v8flags to version 3.0.0 knex/knex#2288

Merged

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

Update v8flags to version 3.0.0 (#2288)

f40d69d

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

Siilwyn mentioned this pull request Oct 9, 2018

XDG compliance yarnpkg/yarn#2334

Open

Siilwyn mentioned this pull request Nov 13, 2018

Follow operating system config location standards vercel/vercel#1276

Closed


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

Save cached config to standard directories #38

Save cached config to standard directories #38

Conversation

Siilwyn commented May 8, 2017

Siilwyn commented May 8, 2017

phated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phated May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Siilwyn May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Siilwyn commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkellen May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Siilwyn commented Jun 19, 2017

phated commented Jun 20, 2017

Siilwyn commented Jun 21, 2017

Siilwyn commented Jun 27, 2017

Siilwyn commented Jul 13, 2017

tkellen commented Jul 14, 2017

Siilwyn commented Jul 14, 2017

Siilwyn May 9, 2017 •

edited

Loading

Siilwyn May 9, 2017 •

edited

Loading

phated May 9, 2017 •

edited

Loading

Siilwyn May 10, 2017 •

edited

Loading

tkellen May 12, 2017 •

edited

Loading

Siilwyn May 20, 2017 •

edited

Loading

Siilwyn Jun 7, 2017 •

edited

Loading