-
Notifications
You must be signed in to change notification settings - Fork 106
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 cache to standard directory #37
Save cache to standard directory #37
Conversation
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? |
Removing Node 0.12 support won't happen until LTS is over for that series. I learned that the hard way. |
I don't suppose there is an |
Unfortunately not, it's ES6 from the start. :*) |
Personally, I'm OK with waiting. In the meantime though, is it possible to add tests, particularly for the cache directory migration behavior? |
Great idea, I discovered my implementation only to be working partially! Now it's all good, added a test. 💡 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few conceptual questions, aside from the CI failure.
lib/index.js
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it should be electron
rather than electron-download
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, added locations to the readme and renamed it to 'electron'.
test/test.js
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ...? Apart from cleaning up the file it does the same as the steps you proposed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it if it was more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright will rewrite it a bit then.
FWIW, |
Think |
Just checking, is this scheduled to be merged once 0.12 is no longer supported (end of the month)? |
That's my plan, unless one of the other maintainers has concerns. |
test/test.js
Outdated
@@ -15,3 +19,31 @@ test('Basic test', (t) => { | |||
t.end() | |||
}) | |||
}) | |||
|
|||
test.only('Cache directory is moved to new location', (t) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .only
call should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, fixed. :D
@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 |
Another alternative is using a module compatible with Node 0.12, if the maintainers don't feel comfortable removing support. |
@TheSBros I mentioned a 0.12-compatible module in #37 (comment), with the response right below it. |
Hey also tuning in. :) |
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`.
Ugh, just tried rebasing, every commit gives a different merge conflict. |
Not sure if I did or not but it seems there are no conflicts with master anymore. :) Any news from the discussion? |
Maintainer update: discussion is still ongoing, I expect to have more information next week. |
Alright, thanks for the quick reply @malept ! |
Any update? Can I read the discussion anywhere or is it a closed discussion? |
@Siilwyn 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? |
Any update on this? |
@kevinsawicki ping |
@kevinsawicki 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 Could we instead just use That way it would be backwards compatible still but we wouldn't have to worry about moving directories around. |
It also looks like there are conflicts here as well that need to be resolved. |
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. |
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:
|
Thank you for your reply Zeke, I just added some fallback code all tests are passing now! |
readme.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be defaults to <user's cache directory>/electron
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sharp eye!
This is a leftover from before the 'Cache location' section got added so I removed it as it's duplicate.
Thanks for this @Siilwyn, apologies for the mixed signals, and appreciate the resiliency 👍 🚀 |
Nice to see this getting merged! :) |
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
.