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

Inconsistency between async and sync implementations #23

Closed
RyanZim opened this issue Feb 7, 2020 · 1 comment · Fixed by #24
Closed

Inconsistency between async and sync implementations #23

RyanZim opened this issue Feb 7, 2020 · 1 comment · Fixed by #24

Comments

@RyanZim
Copy link
Contributor

RyanZim commented Feb 7, 2020

In the sync implementation, we make a statSync call, and if it fails, the original mkdirSync error is thrown:

make-dir/index.js

Lines 137 to 143 in 9de6474

try {
if (!options.fs.statSync(pth).isDirectory()) {
throw new Error('The path is not a directory');
}
} catch (_) {
throw error;
}

However, in the async implementation, we have no error handling for the stat call, and any failure there would bubble up:

make-dir/index.js

Lines 84 to 87 in 9de6474

const stats = await stat(pth);
if (!stats.isDirectory()) {
throw error;
}

In an earlier (pre-async/await) version, we properly swallowed any stat errors, and threw the original error, like the sync implementation:

make-dir/index.js

Lines 76 to 80 in 379001f

return stat(pth)
.then(stats => stats.isDirectory() ? pth : Promise.reject())
.catch(() => {
throw error;
});

I don't have a test case to show if this actually creates an inconsistency for the end user, but it was just noticed in review, and I thought I'd report upstream.

@sindresorhus
Copy link
Owner

Good catch. That should indeed be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants