Hacker News Reader

Comment by lucideer | Hacker News Reader

This is really horrific.

The idea that correctMkdir() exists at all seems to me to be so wrong-headed.

This comment from the source says a lot:

    // annoying humans and their expectations!

Good UX is an important, oft-overlooked consideration, but there is definitely such a thing as taking it too far. If your humans are expecting this level of hand-holding, it's because you've trained them to expect it by pandering to them up until now. This is the kind of problem that should be handled with good, detailed, error message display when users don't get the result they expect, not "fixing" it with over-reaching magic.

I'm not sure I'd trust anything put out by the npm team in general from hereonin if they genuinely thought creating the correct-mkdir.js file in the first place was a reasonable idea. Is it? Genuinely open to a counter-argument.

View on HN

You want to know how this happened?

It all started with ubuntu pointing home to the user's dir when running sudo. This was done out of convenience to have gedit and other Xorg apps work when run with sudo...

Then there is also the terrible fact that ~/.local/bin didn't exist as a "standard" at the time. Which means your only sure-fire non-complicated way to install local bins guaranteed to work for the user was to put them in /usr/local/bin which meant running sudo.

But if you create a package cache dir during sudo in ubuntu in $HOME, thats with root permissions! Then you get errors when trying to run npm without root and it tries to manipulate the cache. How do we fix this, by changing cache dir permissions of course. https://github.com/npm/npm/commit/ebd0b32510f48f5773b9dd2e36...

Through series of refactorings, this became correctMkdirp, which is a non descriptive name of mkdir with (among other things) changing of permissions. And with a name like that it eventually was used in the wrong context and did the wrong thing.

I call this death-by-10-small-missteps. But I would pin the biggest problem on a missing omnipresent `~/.local/bin` standard (at the time). It doesn't cost much if anything, and it would single-handedly obliterate the need for users to much around in their paths (bad usability) or run sudo to install command line tools for personal use (clearly not the best idea).

> But if you create a package cache dir during sudo in ubuntu in $HOME, thats with root permissions! Then you get errors when trying to run npm without root and it tries to manipulate the cache. How do we fix this [...]

    if dir !exists:
      mkdir && chown
    else:
      if dir has correct ownership:
        traverse
      else:
        // our code chowns correctly on create
        // so user must have done something
        // independently; better NOT mess with it
        throw "helpful descriptive message"

</pseudocode>

mkdirp creates OR traverses recursively based on whether each directory already exists or not. This is why correctMkdirp() is an insane idea: the "correct"-ing chown step should never be internal to mkdirp because it should never occur on traversal (i.e. when a pre-existing directory is encountered).

What about tarball extraction and native module build artefacts being produced? They'll still have the wrong permissions.

This was not originally about "mkdirp". It was about managing the cache when running with sudo on ubuntu. It only became a "general" mkdir through series of refactoring steps.

FWIW the comment you're calling out here is four years old: https://github.com/npm/npm/blame/d3095ff20b8ea01e7fbf93a4a69..., before npm inc was formed.

The correctMkdir change seems more recent, but not really related to that specific comment.

This could be my inner grumpy old man speaking, but as a general rule of thumb, I look very poorly on editorializing in code comments. Originally because I didn't want my junior devs embarrassing the company when our clients received control of the code we wrote, but that also transferred into my perception of open source.

That comment should not have survived 4 years. Again, inner grumpy old man showing through.

Edit: to be clear, such comments are treated as reflective of the people and organization behind them.

I can see your point (particularly with regard to deliverables), but I suspect the practice is quite widespread - comments often end up being used as a sort of brain dump.

For instance, see this article (from 2004) on comments in the Win2k source code: http://atdt.freeshell.org/k5/story_2004_2_15_71552_7795.html

That's a fair enough point, especially when directly employed working on closed source / proprietary code. You're essentially stuck in an echo chamber, and professional standards are more difficult to maintain when you don't have the whole of the world looking on.

I also imagine that the mental strain of figuring out edge cases and poor documentation in a system as complex as a windows OS would be enough to make anyone at least a little salty.

However widespread it may be, that does not me that I have to like it :D

I think there should definitely be limits to this—some brevity/levity can be positive—so I would always try to err on the side of acceptance, but in general I agree. In this particular case at least, this comment seems to betray some hint of an anti-user sentiment.

An appropriate limit is, as I mentioned, editorializing. To be precise, your clients, peers and users should not the the target of your feelings expressed in comments.

An additional litmus should be professional discipline: express dissatisfaction with a todo (ideally referencing a bug or discussion issue source URL or identifier). Without that reference, it acknowledges an issue without indicating any motivation to solve or re-mediate the original cause, which is (IMHO) indicative of a careless and lazy attitude.

I think it's even more harmful: "there's the comment that code does X, so the code does X" (or in this case, an implicit hint that the code fixes non-X) - in other words, wishful thinking.

Ah, yeah, you're right. I was looking at it in diff[0] and hadn't noticed it lost context.

With the full comment, it seems they're instead bemoaning having to adhere to a user's config. Not sure which is worse...

[0] https://github.com/npm/npm/commit/94227e15eeced836b3d7b3d2b5...

Seemingly even older: https://github.com/npm/npm/commit/d519f4bd7249cc0c19baca16c3...

I initially parsed that comment as "this library exists to annoy humans" rather than "we wrote this to satisfy humans, who are annoying in this respect".

Reply on news.ycombinator.com