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

feat: return path as name for root directories #36

Closed
wants to merge 1 commit into from
Closed

feat: return path as name for root directories #36

wants to merge 1 commit into from

Conversation

Sebastian-Webster
Copy link

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

What/Why

Previously, name-from-folder would return an empty string when passing in a root directory path (Linux/macOS: / and Windows: {A-Z}:\).

With this PR, instead of returning an empty string, it now returns the path for the directory as the name

References

This change is required for npm/cli#7038 to work.

EDIT: Linked to npm/cli PR

module.exports = dir => dir ? getName(basename(dirname(dir)), basename(dir))
: false
module.exports = (dir) => {
if (!dir) {
Copy link
Member

Choose a reason for hiding this comment

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

Fun fact: removing ternary operators is a good way to get on my good side ;)

Copy link
Author

Choose a reason for hiding this comment

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

Good to know ;)

@wraithgar
Copy link
Member

wraithgar commented Dec 1, 2023

The package name at the root should be empty. Returning the path is incorrect. Your package name is not "/" nor is it "C:\"

@wraithgar
Copy link
Member

Can you show where this is blocking npm/cli#7038? That isn't apparent to me.

@Sebastian-Webster
Copy link
Author

Sebastian-Webster commented Dec 2, 2023

Can you show where this is blocking npm/cli#7038? That isn't apparent to me.

Sure :). As far as my understanding goes, the buildIdealTree method in workspaces/arborist/lib/arborist/build-ideal-tree.js creates an idealTree tracker with no subsection or key, and then #buildDeps in the same file creates an idealTree tracker with a subsection tree.name and an empty string for the key.

The tree.name part comes from the constructor for the Node class in workspaces/arborist/lib/node.js.
Assuming the path you are trying to run npm install at is E:\test, this.name would be set to the name parameter, but name is undefined so it gets the name from @npmcli/name-from-folder (which in this example would return test) and that becomes the value of this.name. In the #buildDeps method in build-ideal-tree a tracker would be created with the subsection being test.

Assuming the path you are trying to run npm install at is E:\, the name parameter in the constructor function is still undefined so it tries getting the name from@npmcli/name-from-folder. The name from folder would be an empty string, and since that is falsey it'll try to get the name from pkg.name. pkg is an empty object in this case, and therefore pkg.name is undefined. Undefined is falsey so it'll make this.name be null. That would make the #buildDeps method try to create an idealTree tracker with subsection null, but the buildIdealTree method has already made an idealTree tracker, so an error is created saying npm ERR! Tracker "idealTree" already exists

The package name at the root should be empty. Returning the path is incorrect. Your package name is not "/" nor is it "C:"

My way to resolve the issue above was to have @npmcli/name-from-folder return the path for root directories since they have no name so then tree.name wouldn't be null and an idealTree tracker could be created. Since that is incorrect behaviour, would it be alright to move the root directory logic to the Node class constructor so this.name would be set to the root directory path if path is a root directory? I wouldn't think it would create any conflicts since slashes can't be used in package names.

@wraithgar
Copy link
Member

I think you're beginning to see why root packages are not allowed. Is the problem you're trying to solve so much of an issue that we have to refactor major parts of npm now? npm has never allowed root directories to be packages. Environments like docker solve this by putting things in a subdirectory first, which is pretty standard at this point.

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

I agree that for macOS and Linux it wouldn't make sense to installing packages in a root directory, but in Windows (excluding the boot drive) it could make some slight sense for installing in the root directory for external drives if the only thing you want on your drive is a JS project. I could make root directory installs only allowed on Windows on drives other than C if your worry is with root directories for boot drives, but if root directory installs shouldn't be allowed regardless of what drive it is being installed in I could remove all of the changes for root installs. The current behaviour of installing packages in a root directory is throwing an error (mkdir error on Windows and Tracker "idealTree" already exists. error in macOS and Linux), and if root directory installs should not be allowed, I think it would be more helpful to the user to show an error saying that root directory installs are not allowed and they should create a subdirectory first. With the current behaviour it is hard to tell how to fix the problem and what the problem is.
I'll close this pull request because even if root directory installs should be allowed for Windows drives that aren't C I've found a way that could achieve that in the main cli repository with a one-line change so this pull request will no longer be needed regardless.

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.

2 participants