-
-
Notifications
You must be signed in to change notification settings - Fork 451
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
fix cache directory not writable (#327) #338
Conversation
Current coverage is 82.35% (diff: 75.00%)@@ master #338 diff @@
==========================================
Files 6 6
Lines 153 153
Methods 22 21 -1
Messages 0 0
Branches 33 31 -2
==========================================
+ Hits 125 126 +1
- Misses 13 14 +1
+ Partials 15 13 -2
|
directory = params.directory; | ||
mkdirp.sync(directory); | ||
} else { | ||
directory = findCacheDir({ name: "babel-loader", create: true }); |
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.
If findCacheDir
doesn't find the package root directory, it doesn't throw, only returns null
. Can you handle that case below? If directory is null
we should fall back to os.tmpdir()
.
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, but passing create: true
makes findCacheDir
create the directory, which will force it to throw if it doesn't exist.
It would make sense to turn this into a test case.
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.
Only if it can find a parent directory with package.json
: https://github.com/avajs/find-cache-dir/blob/7df8ba743347945d1f6d51a281fe6776d656b104/index.js#L17-L19
If pkgDir.sync(dir)
returns null
, findCacheDir
will also return null
regardless of the create
option.
+1 for adding a test case.
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.
You're completely right, I guess I should not let findCacheDir
to create the directory. I'll take a look at this today hopefully.
merged with #346 |
Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
Cannot recover if cache directory is not writable.
What is the new behavior?
Switches to tmpdir if cache directory is not writable.
Does this PR introduce a breaking change?