Save cache to standard directory #37

Siilwyn · 2016-10-15T11:54:10Z

The cache is saved to a standard cache directory using the "env-paths"
module, rather than saving in the home directory.
The old cache directory will automatically be migrated to the new
location.

BREAKING CHANGE: Cache is no longer stored in ~/.electron.

Siilwyn · 2016-11-04T15:04:13Z

The failing tests do not seem related to this but have to do with older Node.js versions. Maybe bundle this change with removing 0.10 and 0.12 support and then do a major version bump?

malept · 2016-11-17T06:42:35Z

Maybe bundle this change with removing 0.10 and 0.12 support and then do a major version bump?

Removing Node 0.12 support won't happen until LTS is over for that series. I learned that the hard way.

malept · 2016-11-17T06:43:45Z

I don't suppose there is an env-paths version (or similar module) that has Node 0.12 support?

Siilwyn · 2016-11-17T08:22:57Z

Unfortunately not, it's ES6 from the start. :*)
I can make a compatible one very easily or we have to wait for next year, your call...

malept · 2016-11-17T14:43:31Z

Personally, I'm OK with waiting. In the meantime though, is it possible to add tests, particularly for the cache directory migration behavior?

Siilwyn · 2016-11-20T10:55:19Z

Great idea, I discovered my implementation only to be working partially! Now it's all good, added a test. 💡

malept

I have a few conceptual questions, aside from the CI failure.

malept · 2016-11-20T22:23:20Z

lib/index.js

@@ -37,7 +38,8 @@ class ElectronDownloader {
  }

  get cache () {
-    return this.opts.cache || path.join(homePath(), './.electron')
+    // use passed argument or XDG environment variable fallback to OS default
+    return this.opts.cache || envPaths('electron-download', {suffix: ''}).cache


I wonder if it should be electron rather than electron-download.

Not sure since this is for the module electron-download thought it would make sense to name it like that... But since it's for electron it might make sense to name it like that.

Yeah, ultimately the electron module uses this. Now that you've reminded me, it's worth documenting where the default cache directory is on Windows/macOS/Linux in the README.

Okay, added locations to the readme and renamed it to 'electron'.

malept · 2016-11-20T22:28:12Z

test/test.js

@@ -15,3 +17,27 @@ test('Basic test', (t) => {
    t.end()
  })
 })
+
+test('Cache directory is moved to new location', (t) => {
+  // Download to old cache directory location


I don't know whether this is the right way to write this test. Perhaps if the user specifies a custom cache directory, the migration code shouldn't be triggered.

Regardless of the decision reached for the above question, I think a better test would be:

Create $HOME/.electron if it doesn't exist, and put a temporary file in that folder

Invoke download()

Assert that the temporary file exists in the new default cache directory

Delete temporary file

🤔 It's only temporary code though just for migrating, after some time it will be removed... Already spent a lot of time on it, apart from cleaning up the file it does the same as the steps you proposed right?

Thoughts on how important this is? Not saying I don't want to do it at all but I'm just considering the time it costs versus value that comes out of it.

I don't know about "temporary". I think the migration code is going to stick around for at least six months after release, since not everyone immediately upgrades.

So ...? Apart from cleaning up the file it does the same as the steps you proposed right?

I would prefer it if it was more explicit.

Alright will rewrite it a bit then.

malept · 2016-12-12T16:38:49Z

FWIW, cachedir provides the same functionality (as of today 😄) and works on Node < 4.

Siilwyn · 2016-12-12T20:28:51Z

Think env-paths is a better fit because it's used in more modules which has multiple advantages. Also a module to only return a path to the cache directory is a bit too minimal for my taste. Thanks for the heads-up though! 2016-12-31 is almost there! :-)

odensc · 2016-12-23T23:23:26Z

Just checking, is this scheduled to be merged once 0.12 is no longer supported (end of the month)?

malept · 2016-12-24T00:14:26Z

That's my plan, unless one of the other maintainers has concerns.

kevinsawicki · 2016-12-27T18:38:09Z

test/test.js

@@ -15,3 +19,31 @@ test('Basic test', (t) => {
    t.end()
  })
 })
+
+test.only('Cache directory is moved to new location', (t) => {


This .only call should be removed.

Whoops, fixed. :D

malept · 2017-01-11T20:42:13Z

@Siilwyn There seems to be a merge conflict.

(If you're wondering why this hasn't gotten merged yet, aside from the merge conflict, the Electron maintainers are taking removal of Node version support very seriously, because this affects the electron module on NPM, and a fair amount of discussion has been generated. We'll know more next week.)

odensc · 2017-01-11T20:52:39Z

Another alternative is using a module compatible with Node 0.12, if the maintainers don't feel comfortable removing support.

malept · 2017-01-11T20:54:02Z

@TheSBros I mentioned a 0.12-compatible module in #37 (comment), with the response right below it.

Siilwyn · 2017-01-11T20:58:22Z

Hey also tuning in. :)
Let's await next week's outcome. I hope they choose to move forward and leave 0.12 behind us, I also suggested writing a 0.12 compatible module earlier. However I think it's better to depend on a wildly used module.

The cache is saved to a standard cache directory using the "env-paths" module, rather than saving in the home directory. The old cache directory will automatically be migrated to the new location. Resolves #24 BREAKING CHANGE: Cache is no longer stored in `~/.electron`.

Siilwyn · 2017-01-11T21:12:29Z

Ugh, just tried rebasing, every commit gives a different merge conflict.
Will look into it later again, reverted the rebase for now.

Siilwyn · 2017-01-19T19:38:33Z

Not sure if I did or not but it seems there are no conflicts with master anymore. :)

Any news from the discussion?

malept · 2017-01-19T19:42:44Z

Maintainer update: discussion is still ongoing, I expect to have more information next week.

Siilwyn · 2017-01-19T19:44:39Z

Alright, thanks for the quick reply @malept !

Siilwyn · 2017-02-02T19:50:28Z

Any update? Can I read the discussion anywhere or is it a closed discussion?

malept · 2017-02-02T19:55:44Z

@Siilwyn
We've agreed to move to drop Node < 4 support (an official announcement will hopefully be in the near future), and so I've opened #45 as a result.

During that discussion, we discussed this PR in particular. Some of the maintainers have reservations about accepting this from a conceptual point of view. @kevinsawicki or @zeke, could one of you chime in so I don't misconstrue what was said during the meeting?

Siilwyn · 2017-03-13T20:23:45Z

Any update on this?

malept · 2017-03-13T20:26:25Z

@kevinsawicki ping

Siilwyn · 2017-03-26T18:53:59Z

@kevinsawicki could you please review this again?

kevinsawicki · 2017-03-27T16:25:11Z

could you please review this again?

I'm not a huge fan of moving a directory each time this runs. Other apps or libraries could be using ~/.electron and it would seem confusing and hard to track down why this directory is disappearing each time you npm install electron.

Could we instead just use ~/.electron as a fallback location if the file isn't in the new cache location and write any new files to the new location?

That way it would be backwards compatible still but we wouldn't have to worry about moving directories around.

kevinsawicki · 2017-03-27T16:27:12Z

It also looks like there are conflicts here as well that need to be resolved.

Siilwyn · 2017-03-29T14:11:55Z

Sounds good to me, will remove the migration code and when I have some time later this week I'll add the fallback logic.

I don't want to be pointing fingers and get negative but would really like to get this off my chest: the migration of the directory was suggested to me and I spent a lot of time getting that working and covered by a test. Solving the merge conflicts is something that happened multiple times now on this PR because of the slow responses. Overall it would be beneficial for 'other' contributors if core contributors are on one line.

zeke · 2017-03-29T17:01:29Z

Sorry to leave this hanging for so long @Siilwyn, and thank you for patiently rebasing and making suggested changes.

I see one failure on Travis:

not ok 17 Zip path should end with -dsym.zip
  ---
    operator: ok
    expected: true
    actual:   false
    at: download (/Users/travis/build/electron-userland/electron-download/test/test_dsym.js:16:7)

Siilwyn · 2017-03-30T10:28:35Z

Thank you for your reply Zeke, I just added some fallback code all tests are passing now!

kevinsawicki · 2017-03-30T18:03:08Z

readme.md

@@ -25,7 +25,7 @@ download({
  version: '0.25.1',
  arch: 'ia32',
  platform: 'win32',
-  cache: './zips' // defaults to <user's home directory>/.electron
+  cache: './zips' // defaults to <user's cache directory>/electron-download


Should this be defaults to <user's cache directory>/electron instead?

Yes, sharp eye!
This is a leftover from before the 'Cache location' section got added so I removed it as it's duplicate.

kevinsawicki · 2017-03-30T22:48:54Z

Thanks for this @Siilwyn, apologies for the mixed signals, and appreciate the resiliency 👍 🚀

Siilwyn · 2017-04-02T08:12:14Z

Nice to see this getting merged! :)

malept reviewed Nov 20, 2016

View reviewed changes

odensc mentioned this pull request Dec 23, 2016

Use standard cache directories electron-userland/electron-prebuilt#223

Closed

kevinsawicki suggested changes Dec 27, 2016

View reviewed changes

Siilwyn added 5 commits January 11, 2017 22:02

Rewrite migration test

951f670

Rename new location to electron

547d47a

Describe default cache location

a59278d

Remove development leftover

f9be647

malept mentioned this pull request Jan 20, 2017

Add an electron-forge clean command electron/forge#92

Closed

Merge branch 'master' into 24-follow-os-standards

db92aa9

Siilwyn added 2 commits March 29, 2017 21:42

Remove migration code

4b2b22f

Fallback to old cache location

803f90c

zeke approved these changes Mar 30, 2017

View reviewed changes

kevinsawicki reviewed Mar 30, 2017

View reviewed changes

Siilwyn added 2 commits March 30, 2017 20:35

Remove duplicate reference

7ff0103

Update env-paths dependency to 1.0.0

18c33a9

kevinsawicki approved these changes Mar 30, 2017

View reviewed changes

kevinsawicki merged commit e1a1fe0 into electron:master Mar 30, 2017

Siilwyn mentioned this pull request Apr 15, 2017

Fix fallback location #54

Merged

kevinsawicki mentioned this pull request May 10, 2017

Remove fallback location and document how to migrate #55

Closed

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

Save cache to standard directory #37

Save cache to standard directory #37

Conversation

Siilwyn commented Oct 15, 2016

Siilwyn commented Nov 4, 2016

malept commented Nov 17, 2016

malept commented Nov 17, 2016

Siilwyn commented Nov 17, 2016

malept commented Nov 17, 2016

Siilwyn commented Nov 20, 2016

malept left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept commented Dec 12, 2016

Siilwyn commented Dec 12, 2016 • edited Loading

odensc commented Dec 23, 2016

malept commented Dec 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept commented Jan 11, 2017

odensc commented Jan 11, 2017

malept commented Jan 11, 2017

Siilwyn commented Jan 11, 2017

Siilwyn commented Jan 11, 2017

Siilwyn commented Jan 19, 2017 • edited Loading

malept commented Jan 19, 2017

Siilwyn commented Jan 19, 2017

Siilwyn commented Feb 2, 2017 • edited Loading

malept commented Feb 2, 2017

Siilwyn commented Mar 13, 2017

malept commented Mar 13, 2017

Siilwyn commented Mar 26, 2017

kevinsawicki commented Mar 27, 2017

kevinsawicki commented Mar 27, 2017

Siilwyn commented Mar 29, 2017 • edited Loading

zeke commented Mar 29, 2017

Siilwyn commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinsawicki commented Mar 30, 2017

Siilwyn commented Apr 2, 2017

Siilwyn commented Dec 12, 2016 •

edited

Loading

Siilwyn commented Jan 19, 2017 •

edited

Loading

Siilwyn commented Feb 2, 2017 •

edited

Loading

Siilwyn commented Mar 29, 2017 •

edited

Loading