Skip to content

Commit

Permalink
don't use rimraf to remove a file
Browse files Browse the repository at this point in the history
  • Loading branch information
BYK committed Sep 6, 2017
1 parent 5685843 commit 9576dbd
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/util/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export const rename: (oldPath: string, newPath: string) => Promise<void> = promi
export const access: (path: string, mode?: number) => Promise<void> = promisify(fs.access);
export const stat: (path: string) => Promise<fs.Stats> = promisify(fs.stat);
export const unlink: (path: string) => Promise<void> = promisify(require('rimraf'));
export const unlinkFile: (path: string) => Promise<void> = promisify(fs.unlink);
export const mkdirp: (path: string) => Promise<void> = promisify(require('mkdirp'));
export const exists: (path: string) => Promise<boolean> = promisify(fs.exists, true);
export const lstat: (path: string) => Promise<fs.Stats> = promisify(fs.lstat);
Expand Down Expand Up @@ -903,7 +904,7 @@ export async function getFirstWriteableFolder(paths: Iterable<string>): Promise<
const testFile = path.join(folder, `.yarn-write-test-${process.pid}`);
await writeFile(testFile, '');
await readFile(testFile);
await unlink(testFile);
await unlinkFile(testFile);

result.folder = folder;

Expand Down

5 comments on commit 9576dbd

@arcanis
Copy link
Member

@arcanis arcanis commented on 9576dbd Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. The perf doesn't worth the added complexity imo.

@BYK
Copy link
Member Author

@BYK BYK commented on 9576dbd Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was because rimraf throwing an assertion error, not to improve performance. I agree with you on that part ☺️

@arcanis
Copy link
Member

@arcanis arcanis commented on 9576dbd Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird, we're using unlink to delete files everywhere in the code 😯

For example here: https://github.com/yarnpkg/yarn/blob/master/src/fetchers/tarball-fetcher.js#L192

@BYK
Copy link
Member Author

@BYK BYK commented on 9576dbd Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. I think I hit the same error you've faced when working on the cache fallback code. This failed only on AppVeyor.

@arcanis
Copy link
Member

@arcanis arcanis commented on 9576dbd Sep 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh .. I think the issue then was that Windows was locking the newly created file for a short time. That's why I eventually removed the unlink altogether, since it's not required strictly speaking and we're only writing in an internal directory anyway.

Please sign in to comment.