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

os: make tmpdir be const value #1648

Closed
wants to merge 1 commit into from
Closed

os: make tmpdir be const value #1648

wants to merge 1 commit into from

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented May 6, 2015

This patch also make the function getTmpdir() be called once with a little improvement, but the main purpose of this PR is to make tmpdir as const as well.

@mscdex mscdex added the os Issues and PRs related to the os subsystem. label May 6, 2015
@brendanashworth
Copy link
Contributor

Couldn't os.tmpdir() change during program execution if the user changes one of the environment variables?

@jbergstroem
Copy link
Member

@brendanashworth I recall python's tempfile module using the same assumption (read env on every call). I think we should avoid making this a const.

@Fishrock123 Fishrock123 added the discuss Issues opened for discussions and feedbacks. label May 7, 2015
@silverwind
Copy link
Contributor

With environment variables read only once at startup, it should currently behaves as a constant, unless the user changes the variables during runtime (unlikely), which we would take away the ability to do so.

@ChALkeR
Copy link
Member

ChALkeR commented May 7, 2015

@silverwind Could a variable in process.env be changed from outside of the js code while the iojs process is running?

@targos
Copy link
Member

targos commented May 7, 2015

@ChALkeR yes it can. process.env[name] calls getenv.
But as @silverwind said it is very unlikely that the environment variables will change for the running process.

@domenic
Copy link
Contributor

domenic commented May 7, 2015

It is a function, after all: os.tmpdir(), not os.tmpdir. As such I think it's pretty reasonable to expect it will do some work every time you call it, and be always up to date. If people want a const value they can just do const t = os.tmpdir().

@silverwind
Copy link
Contributor

The process gets a snapshot of the environment on startup and the only way for it to change is through assignment to process.env.TMP and similar variables from within the process. This PR would take that option away from the user.

@ChALkeR
Copy link
Member

ChALkeR commented May 7, 2015 via email

@vkurchatkin
Copy link
Contributor

-1. In most usage patterns it is cached anyway

@Fishrock123
Copy link
Contributor

Closing for now, seems like the consensus is this isn't too great, and you can cache it yourself where applicable in programs. :)

gcanti pushed a commit to gcanti/io-ts that referenced this pull request Nov 28, 2017
Just a small documentation update, if you're open to it. Suggesting that `NODE_ENV` be "cached" in the example `unsafeValidate` method.

Reasoning / discussions here:
* nodejs/node#3104
* nodejs/node#1648

Unless you'd expect `NODE_ENV` to be editable while the application is running (debugging on a live server, perhaps?)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants