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

doc: update os.tmpdir() to discuss symlink results on macOS #31877

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 20, 2020

Closes: #11422

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. os Issues and PRs related to the os subsystem. labels Feb 20, 2020
@bnoordhuis
Copy link
Member

I'm -.5 on documenting a platform quirk that 99% of the time should be inconsequential.

sindresorhus/temp-write#8 seems to be about writing Node.js code to the temp dir, then loading that with require().

Yes, __filename will be different from what you might naively expect - but take a step back and ask yourself if you're doing something that maybe you shouldn't be doing in the first place.

@fabiospampinato
Copy link

I'm -.5 on documenting a platform quirk

I think I'd be better to remove the quirk entirely.

And there's value in documenting these quirks (https://github.com/ehmicky/cross-platform-node-guide).

99% of the time should be inconsequential.

Probably even higher than that, but isn't it worth something making this less quirky?

@addaleax
Copy link
Member

I'm -.5 on documenting a platform quirk

I think I'd be better to remove the quirk entirely.

@fabiosantoscode What is the quirk here exactly, in your opinion? I’d consider it odd if Node.js did silently resolve symlinks for the temp directory.

@fabiospampinato
Copy link

@addaleax that Node under macOS returns a symlink to the temp directory rather than its actual path as it does under other OS', or at least I think that's how it currently works since this isn't documented.

Why would you consider it odd if Node always returned the actual path to the temp directory across all OS'?

@addaleax
Copy link
Member

@addaleax that Node under macOS returns a symlink to the temp directory rather than its actual path as it does under other OS', or at least I think that's how it currently works since this isn't documented.

If another OS reports its temporary directory as a symlink, then Node.js will also report that there.

Why would you consider it odd if Node always returned the actual path to the temp directory across all OS'?

Because the concept of an “actual path” is a bit odd – yes, you can resolve symlinks, but you shouldn’t really have to. The whole idea of symlinks is that you can use them transparently instead of whatever path they point to.

If it’s about the scenario described by Ben above, writing Node.js code to disk and then running it, then yes, Node.js does something weird – but it does so in the module loader, where it uses fs.realpath(), not when it returns the temporary directory as a symlink.

(I, personally, would still like to see a solution for that issue along the lines of nodejs/node-eps#46 (comment) – but that’s out of scope for this PR here, I’d say…)

@fabiospampinato
Copy link

If another OS reports its temporary directory as a symlink, then Node.js will also report that there.

Yes I understand that's what's happening here.

My point is that Node smooths over other differences already, and this looks like another small difference I'd expect it to smooth over too.

Because the concept of an “actual path” is a bit odd – yes, you can resolve symlinks, but you shouldn’t really have to. The whole idea of symlinks is that you can use them transparently instead of whatever path they point to.

I don't understand what's your point, are you arguing for the removal of fs.realpath because it shouldn't really be used? Are you saying that the people who may stumble upon cross-platform compatibility issues because Node sometimes is returning a symlink to the OS' "actual" temp dir should do things differently or that Node itself should do things differently elsewhere in order to avoid these potential issues?

If it’s about the scenario described by Ben above

I don't actually have a material use case for the change I'm arguing for, other than pushing for Node to be less quirky and reducing my bundles by a tiny amount since what temp-dir or similar packages do won't be required anymore I guess.

@addaleax
Copy link
Member

Because the concept of an “actual path” is a bit odd – yes, you can resolve symlinks, but you shouldn’t really have to. The whole idea of symlinks is that you can use them transparently instead of whatever path they point to.

I don't understand what's your point, are you arguing for the removal of fs.realpath because it shouldn't really be used?

No. (But I am unhappy about the fact that Node.js uses it in its module loader because it shouldn’t, yes.)

Are you saying that the people who may stumble upon cross-platform compatibility issues because Node sometimes is returning a symlink to the OS' "actual" temp dir should do things differently or that Node itself should do things differently elsewhere in order to avoid these potential issues?

The former – the question is, why do you need the resolved path to the directory? The point of symlinks is that you shouldn’t have to.

@fabiospampinato
Copy link

The former – the question is, why do you need the resolved path to the directory? The point of symlinks is that you shouldn’t have to.

Shouldn't !== don't.

I personally have never needed to know about this quirk, but perhaps a reasonable hypothetical scenario that breaks because of this could be the following:

  1. Let's say I have a library that allows me to watch paths for filesystem events
  2. For whatever reasons it doesn't follow symlinks
  3. I want to use this library to watch for filesystem events inside the OS' temporary directory
  4. Now if I'm running this under Windows or Linux it works, but if I'm running it under macOS I will be watching a symlink, and events happening inside the path it resolves to are not reported by the library

Is this a reasonable-enough use case for either smoothing over this quirk or documenting it?

@addaleax
Copy link
Member

@fabiospampinato That seems very hypothetical, and even if it happened, it doesn’t seem like that would be a common use case?

And, again, I don’t see this as a “quirk”. But even if I did agree that there was a problem, there’s still the technical issue that Node.js shouldn’t silently perform sync fs operations under the hood, as mentioned in the linked issue.

@fabiospampinato
Copy link

fabiospampinato commented Feb 20, 2020

@addaleax I can't imagine it being a common use case, given how calling os.tmpdir itself probably isn't that common either, but it seems like a relatively reasonable scenario to me, watching a directory is pretty common, and the temp dir is a directory (well, kinda), besides I don't think this is relevant as I don't think Node is supposed to work for common use cases only.

And, again, I don’t see this as a “quirk”.

Name it what you like, the important bit is that because Node doesn't smooth over this little difference things may unexpectedly not work across platforms, and the reason why they may not work across platforms isn't documented either.

But even if I did agree that there was a problem

Wait do you agree or not that in my hypothetical example above we have a problem?

there’s still the technical issue that Node.js shouldn’t silently perform sync fs operations under the hood, as mentioned in the linked issue.

I can't see any references to this in the linked issues, I'd be interested in hearing the reasoning behind this if anybody can give me a pointer to this, as I struggle to see why this would be a problem, i.e. temp-dir does this and I can't spot the issue with doing it.

Replied here: #11422 (comment)

@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 20, 2020

@fabiospampinato If you think it's a quirk worth addressing, why don't you petition Apple to move away from symlinks? That would fix it for everyone everywhere.

@fabiospampinato
Copy link

@bnoordhuis because I feel it would be 100% a waste of my time, while here I feel there's a non-100% chance I'm wasting my time.

@Trott Trott force-pushed the update-symlink-docs branch from c0b90c4 to cde3551 Compare February 22, 2020 03:44
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Making my objection explicit.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

Given the objections that have been raised and the lack of further activity, closing this. It can be reopened if it is picked back up again.

@jasnell jasnell closed this Jun 19, 2020
@Trott Trott deleted the update-symlink-docs branch April 14, 2022 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On macOS, os.tmpdir() is not a real path
6 participants