Fix fallback location #54
Conversation
|
So my previous PR introduced a 🐛 that causes an error when trying to download a new version that is cached in the original location. |
This fixes caching versions later than 1.3.2 that are using checksum validation.
|
Are we really need to worry about old cache? |
lib/index.js
Outdated
| @@ -44,6 +44,11 @@ class ElectronDownloader { | |||
| } | |||
|
|
|||
| get cache () { | |||
| const oldCacheDirectory = path.join(os.homedir(), './.electron') | |||
| if (pathExists.sync(path.join(oldCacheDirectory, this.filename))) { | |||
There was a problem hiding this comment.
I think, we should avoid sync.
There was a problem hiding this comment.
Async code is often better but I chose for a sync method in this case. Firstly because the path to the cache is needed for all other functionality. Secondly all other code expects this method to be sync. Lastly this fallback will be removed in the future so rewriting other methods to work in an async way with get cache sounds unreasonable.
|
@develar see the original PR and in particular this comment on why we worry about the old cache. |
|
Sorry I forgot to tag @malept who was also involved in the previous PR. |
|
@kevinsawicki thoughts? |
lib/index.js
Outdated
| if (pathExists.sync(path.join(oldCacheDirectory, this.filename))) { | ||
| return path.join(os.homedir(), './.electron') | ||
| } | ||
|
|
||
| // use passed argument or XDG environment variable fallback to OS default | ||
| return this.opts.cache || envPaths('electron', {suffix: ''}).cache |
There was a problem hiding this comment.
Shouldn't this method always return this.opts.cache when specified? Even if the old directory exists.
There was a problem hiding this comment.
Sharp! Added a test to confirm this is working. Also found the first test could be improved.
test/test_fallback.js
Outdated
| Object.assign( | ||
| {}, | ||
| downloadOptions, | ||
| { cache: oldCachePath } |
There was a problem hiding this comment.
Is there a reason for the three objects here instead of just setting cache directly on downloadOptions?
There was a problem hiding this comment.
This is because I re-use the same settings object with the only difference being the cache property. It looks a bit wonky because of the newlines though, removed those. Thanks for your review!
lib/index.js
Outdated
|
|
||
| const oldCacheDirectory = path.join(os.homedir(), './.electron') | ||
| if (pathExists.sync(path.join(oldCacheDirectory, this.filename))) { | ||
| return path.join(os.homedir(), './.electron') |
There was a problem hiding this comment.
This could just return oldCacheDirectory I believe.
|
Thanks @Siilwyn for fixing this 👍 🚢 |
|
Thanks for your helpful reviews too @kevinsawicki! 🎉 |
See @develar's concerns at the original issue #24 and at the atomic rename PR.
I have included a test that fails against master and is passing by moving the fallback logic to directory level.