3 ms·

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: // annoyi

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.

jergason 9y ago
FWIW the comment you're calling out here is four years old: https://github.com/npm/npm/blame/d3095ff20b8ea01e7fbf93a4a697a04fea77d8e6/lib/config/core.js#L153 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.
- zdragnar 9y ago
  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.
  - lucideer 9y ago
    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.
    - zdragnar 9y ago
      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.
  - predakanga 9y ago
    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 http://atdt.freeshell.org/k5/story_2004_2_15_71552_7795.html
    - zdragnar 9y ago
      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
  - Piskvorrr 9y ago
    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.
- lucideer 9y ago
  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/94227e15eeced836b3d7b3d2b5e5cc41d4959cff https://github.com/npm/npm/commit/94227e15eeced836b3d7b3d2b5...
- plopz 9y ago
  Seemingly even older: https://github.com/npm/npm/commit/d519f4bd7249cc0c19baca16c3b6c830413a38c8#diff-0274417318d45b56e7adc8820b6a76b3R101 https://github.com/npm/npm/commit/d519f4bd7249cc0c19baca16c3...
SilasX 9y ago
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".
spion 9y ago
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/ebd0b32510f48f5773b9dd2e36ffc0bf0ecf4768 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).
- lucideer 9y ago
  > 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 [...] <pseudocode> 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).
  - spion 9y ago
    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.

jergason 9y ago

FWIW the comment you're calling out here is four years old: https://github.com/npm/npm/blame/d3095ff20b8ea01e7fbf93a4a697a04fea77d8e6/lib/config/core.js#L153 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.

zdragnar 9y ago
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.
- lucideer 9y ago
  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.
  - zdragnar 9y ago
    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.
- predakanga 9y ago
  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 http://atdt.freeshell.org/k5/story_2004_2_15_71552_7795.html
  - zdragnar 9y ago
    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
- Piskvorrr 9y ago
  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.
lucideer 9y ago
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/94227e15eeced836b3d7b3d2b5e5cc41d4959cff https://github.com/npm/npm/commit/94227e15eeced836b3d7b3d2b5...
plopz 9y ago
Seemingly even older: https://github.com/npm/npm/commit/d519f4bd7249cc0c19baca16c3b6c830413a38c8#diff-0274417318d45b56e7adc8820b6a76b3R101 https://github.com/npm/npm/commit/d519f4bd7249cc0c19baca16c3...

SilasX 9y ago

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".

spion 9y ago

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/ebd0b32510f48f5773b9dd2e36ffc0bf0ecf4768 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).

lucideer 9y ago
> 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 [...] <pseudocode> 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).
- spion 9y ago
  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.