add checks before writing and reading cache #158
Conversation
index.js
Outdated
| fs.writeFileSync(cachePath, JSON.stringify(cache)) | ||
| var cacheJson = JSON.stringify(cacheJson) | ||
| try { | ||
| safeWriteCache(cachePath) |
There was a problem hiding this comment.
The function definition says it accepts cacheJson but you're passing cachePath.
This is a bit confusing—it's hard to tell what is local and what is global, and why.
Can you avoid shared mutable state wherever possible?
index.js
Outdated
| @@ -181,7 +193,7 @@ module.exports = function(input, map) { | |||
| thunk: true, | |||
| create: true, | |||
| }) | |||
| cachePath = thunk("data.json") || os.tmpdir() + "/data.json" | |||
| cachePath = thunk("data.json") || cacheFallback | |||
There was a problem hiding this comment.
Both this line and the previous line (where you declare thunk) can throw.
You need to move them inside a try/catch and apply the same strategy of fallback.
Right now || cacheFallback is not doing anything because if something's bad, the function will throw rather than return null.
viankakrisna
force-pushed
the
patch-1
branch
from
513425e
to
e3bd92b
Compare
index.js
Outdated
| safeWriteCache(cacheJson) | ||
| } | ||
| catch (e) { | ||
| cache = false |
There was a problem hiding this comment.
Why are you using false here? Does it have a special meaning compared to null elsewhere?
index.js
Outdated
| thunk: true, | ||
| create: true, | ||
| }) | ||
| cachePath = thunk("data.json") | ||
| cache = require(cachePath) | ||
| } | ||
| catch (e) { |
There was a problem hiding this comment.
I presume that we want to read from fallback below? Maybe extract it to safeReadCache?
viankakrisna
force-pushed
the
patch-1
branch
from
e3bd92b
to
2aabde7
Compare
index.js
Outdated
| @@ -6,11 +6,13 @@ var fs = require("fs") | |||
| var findCacheDir = require("find-cache-dir") | |||
| var objectHash = require("object-hash") | |||
| var os = require("os") | |||
| var path = require("path") | |||
|
|
|||
| var engines = {} | |||
| var rules = {} | |||
| var cache = null | |||
| var cachePath = null | |||
There was a problem hiding this comment.
Why does this need to be a global variable at all?
index.js
Outdated
| fs.writeFileSync(cachePath, cacheJson) | ||
| } | ||
| else { | ||
| fs.mkdirSync(path.dirname(cachePath)) |
There was a problem hiding this comment.
It is odd that we're using two different ways to create a directory: mkdirSync and findCacheDir (which internally uses mkdirp).
Can you use mkdirp here too? You also don't need these existsSync checks because mkdirp already handles all of that.
viankakrisna
force-pushed
the
patch-1
branch
2 times, most recently
from
4636c39
to
bab165e
Compare
viankakrisna
force-pushed
the
patch-1
branch
from
bab165e
to
6ab09b5
Compare
viankakrisna
changed the title
viankakrisna
changed the title
viankakrisna
changed the title
So the behavior would be similar to findCacheDir
index.js
Outdated
| } | ||
| catch (e) { | ||
| // Maybe permission denied? | ||
| // Don't log errors, try it again in the next lint... |
There was a problem hiding this comment.
so if it always fail, people will never have the cache and no message?
|
I think we should use the Babel version, it already handles edge cases well and IMO is written more consistently |
|
So we should close in favor of #159? |
|
Or maybe you want this merged/released asap? |
|
Let's do #159 instead. |
fix for facebook/create-react-app#1656