Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 26 additions & 25 deletions src/fs-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,35 +125,36 @@ module.exports = function(params, callback) {
const identifier = params.identifier;
let directory;

if (typeof params.directory === "string") {
directory = params.directory;
} else {
directory = findCacheDir({ name: "babel-loader" }) || os.tmpdir();
try {
if (typeof params.directory === "string") {
directory = params.directory;
mkdirp.sync(directory);
} else {
directory = findCacheDir({ name: "babel-loader", create: true });
Copy link
Contributor

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().

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

}
} catch (e) {
// Make sure the directory exists and is writable.
directory = os.tmpdir();
}

const file = path.join(directory, filename(source, identifier, options));

// Make sure the directory exists.
return mkdirp(directory, function(err) {
if (err) { return callback(err); }

return read(file, function(err, content) {
let result = {};
// No errors mean that the file was previously cached
// we just need to return it
if (!err) { return callback(null, content); }

// Otherwise just transform the file
// return it to the user asap and write it in cache
try {
result = transform(source, options);
} catch (error) {
return callback(error);
}

return write(file, result, function(err) {
return callback(err, result);
});
return read(file, function(err, content) {
let result = {};
// No errors mean that the file was previously cached
// we just need to return it
if (!err) { return callback(null, content); }

// Otherwise just transform the file
// return it to the user asap and write it in cache
try {
result = transform(source, options);
} catch (error) {
return callback(error);
}

return write(file, result, function(err) {
return callback(err, result);
});
});
};