fix: don't mkdir directories that already exist #7899
Conversation
Resolution attempt for npm#6412. Unfortunately, resolution of that issue introduces another error; `Tracker "idealTree" already exists`.
wraithgar
changed the title
npm i attempts to mkdir root directories on Windows
Addressed style and syntax issues as per GitHub Workflows feedback.
Now using `access` from `node:fs/promises` instead of `existsSync` from `node:fs`.
|
|
Now using single-quotes instead of double-quotes.
|
Ah, it appears I accidentally used double-quotes instead of single-quotes for the string. |
|
Oh shoot, sorry I forgot this was the cli repo. Yeah you gotta either be in the arborist directory or specify the workspace. I'll have to remember that when suggesting that in the future. |
| await access(resolvedPath) | ||
| } catch (accessError) { | ||
| if (accessError.code === 'ENOENT') { | ||
| await mkdir(resolvedPath, { recursive: true }) |
There was a problem hiding this comment.
Just a note for future devs looking at this change. Any error other than ENOENT that access gets is going to also be thrown if attempting mkdir. I think only ignoring that error is the correct one.
| if (accessError.code === 'ENOENT') { | ||
| await mkdir(resolvedPath, { recursive: true }) | ||
| } else { | ||
| throw accessError |
There was a problem hiding this comment.
The reason tests are failing is because there aren't any that test for this scenario, i.e. an error other than ENOENT happening.
There was a problem hiding this comment.
Will any changes have to be made to this code to resolve the failing tests in this case?
There was a problem hiding this comment.
A test will have to be added to cover that code path.
This pull request aims to resolve #6412, by making checks for whether a project's working directory already exists, before attempting to
mkdirit.This effectively prevents
reify.jsfrom accidentally executingmkdiron root drive letters on Windows, likemkdir "D:/"for example, as the Windows operating system prohibits such operations.The resolution of #6412 will inevitably introduce the
npm error Tracker "idealTree" already existsissue, however I believe that is an issue separate from #6412, and that separate issue was already documented before, #7596 being a good example of one of those documentations (in my opinion).As can be seen in #6412, this pull request makes changes to the following line of code in
reify.js.Of course, should one attempt to execute
npm iin a drive letter that does not exist, then this error in my opinion should take place, informing the user that said drive letter and the path within cannot be created, and that error will take place. However this error in my opinion should not take place on existing drives, hence this pull request.Additional info
/workspaces/arborist/lib/arborist/reify.jsline126npm itries to recreate root directory (drive letter) in Windows #6412