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: allow package installs in drive root directories #7038

Closed
wants to merge 2 commits into from
Closed

fix: allow package installs in drive root directories #7038

wants to merge 2 commits into from

Conversation

Sebastian-Webster
Copy link

@Sebastian-Webster Sebastian-Webster commented Nov 30, 2023

What/Why

Previously trying to do npm install in a drive's root directory (e.g C:\ or E:\ in Windows or / in Linux or macOS) would fail. See screenshots:

Ubuntu

Ubuntu Error Console Log

Windows

Windows Error Console Log

This PR allows users to install npm packages in their drive root directories

What does this PR do and why are these changes being made

The change in workspaces/aborist/lib/aborist/reify.js's _validatePath method was made to check if the project directory exists before creating it. Previously if you were running npm install inside of the drive's root directory (in this example, the E drive), mkdir would get called with path E:/ and it would try to create a directory at the path E:\\ which is invalid, and so it would fail. In this example, with this PR, E:\ already exists so it is not needed to be created, and thus no directory is tried to be created at E:\\.

2 tests were added to test/lib/commands/install.js.
The first one only runs on Windows and tests to see that installs in a root directory work by dry running npm install in C:\
The second one runs on platforms that are not Windows and tests to see that installs in a root directory work by dry running npm install in /

@ljharb
Copy link
Contributor

ljharb commented Nov 30, 2023

While it may be a bug - why would you want to do that?

@Sebastian-Webster
Copy link
Author

It isn't so applicable in Linux or macOS since external drives don't have their own individual root directories but I know a few people that have tried working on a JS project on a USB or external drives in Windows and it failed because of the mkdir issue so it would be a lot more helpful for people using Windows.

@ljharb
Copy link
Contributor

ljharb commented Nov 30, 2023

Ah, so maybe not for C: but for external drives, that makes much more sense - thanks.

@Sebastian-Webster
Copy link
Author

No problem :). I only did C:\\ in the install command test because I wasn't sure what other drive letters would be available in the GitHub Actions runners but I knew C would definitely be there.

@wraithgar
Copy link
Member

As mentioned in npm/name-from-folder/pull/36 I'm hesitant to go down this path, frankly. I think this is a good restriction and keeps a lot of odd (and maybe even potentially dangerous) setups from happening.

@Sebastian-Webster
Copy link
Author

As mentioned in npm/name-from-folder#36 I can agree that package installs in a macOS or Linux root directory wouldn't make much sense, but there can be some sense for Windows drives (excluding C drive) as if you have an external drive on Windows and you only want to have your JS project on it then you may want to just install the packages in the root directory. I don't use Windows so I haven't had that situation happen to me before but I know of a few people who have tried doing that before and got an npm error.

If your issue is only with boot drive root directories, I could make changes to only allow installs in non boot drive directories. Since the behaviour in npm/name-from-folder#36 was incorrect for name-from-folder I have since closed that pull request and could make that change in this repository without needing changes from name-from-folder.

Right now when installing packages in a root directory npm gives you one of two errors. Tracker "idealTree" already exists if you are on macOS or Linux, or a mkdir issue if you are on Windows. If root directory installs should not be allowed, I think an easy to understand error should be shown telling the user that they cannot install packages in a root directory and that they should create a subdirectory first. I could make that change in a new pull request or this one, whichever would be best.

@wraithgar
Copy link
Member

This is a similar problem to what is experience in docker containers, and it is solved by the convention of putting your code in a folder like /app. It's a good convention, especially in thumb drives where there is a much more restricted limit on the number of files you can have in the root, versus in a subdirectory. It's just all around best not to put things in the root folder.

@Sebastian-Webster
Copy link
Author

Oh okay, that makes sense. I'll be closing this pull request then. Thanks :)

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

Successfully merging this pull request may close these issues.

3 participants