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

Mention case-insensitivity in process.env docs #9157

Closed
oliversalzburg opened this issue Oct 18, 2016 · 12 comments
Closed

Mention case-insensitivity in process.env docs #9157

oliversalzburg opened this issue Oct 18, 2016 · 12 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.

Comments

@oliversalzburg
Copy link
Contributor

On Windows OS, environment variables are case-insensitive.

14:46 $ node
> process.env.foo = "lol"
'lol'
> process.env.fOo
'lol'

I think it could be beneficial to make a note about this in the docs.

@addaleax addaleax added doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform. good first issue Issues that are suitable for first-time contributors. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. and removed windows Issues and PRs related to the Windows platform. labels Oct 18, 2016
@addaleax
Copy link
Member

It might also be good to add tests for this behaviour to detect platform differences or keep us from accidentally changing anything here.

Would you be interested in writing a pull request to add this yourself? The files to edit should be doc/api/process.md and/or test/parallel/test-process-env.js.

@oliversalzburg
Copy link
Contributor Author

@addaleax I'm not quite sure how to properly test this behavior, as the behavior would likely change depending on which OS you're running the test on. If you have any suggestions, please let me know and I'd be happy to write the test as well.

@addaleax
Copy link
Member

addaleax commented Oct 18, 2016

@oliversalzburg There’s https://github.com/nodejs/node/blob/master/doc/guides/writing_tests.md for generic “How to write a test” things, and you can check for common.isWindows in tests to see whether the test is running on Windows or not. Ideally, you’d check that other OS show the opposite behaviour, to make sure that it’s really only Windows that does this.

@oliversalzburg
Copy link
Contributor Author

@addaleax Ah, sounds good. Thanks for the help :)

@oliversalzburg
Copy link
Contributor Author

@addaleax Is there also a guide for building Node on Windows? Because I'm getting a bunch of linker errors when trying to vcbuild test nosign with Visual Studio 2015.

@addaleax
Copy link
Member

I am afraid I can only ask @nodejs/platform-windows, but it might help if you have the errors somewhere in a gist or something

@oliversalzburg
Copy link
Contributor Author

Alright, I'll put the information into the PR. Thanks again

@joaocgreis
Copy link
Member

For reference, guide for building on Windows: https://github.com/nodejs/node/blob/master/BUILDING.md#windows

@nfischer
Copy link

So does this mean that process.env is guaranteed to have case-insensitive keys on Windows, even going back to v4? I've hunted in the past for documentation, couldn't find anything, so I just assumed that was the case. Thanks.

@oliversalzburg
Copy link
Contributor Author

@nfischer This behavior is present in Node 4 (and probably earlier as well). We initially noticed this in 4 and assumed it would be something that had changed between 4 and 6. We then realized that it had nothing to do with the version, but with us moving the code from Windows to UNIX.

Also note that this is not really a Node-specific thing. This is simply how Windows handles environment variables.

@nfischer
Copy link

I understand this is how Windows does env variables. I just wanted to verify that the NodeJS API for accessing those env variables is likewise case-insensitive (as opposed to not guaranteeing if the key is "PATH" or "Path" or "path"). Case insensitive is the only thing that makes sense to me, but it's nice to have the guarantee 😄

I would hate to rely on this being case-insensitive, only to have it crash on someone else's Windows setup because it was undefined behavior. It would also be nice to know if this is intended as a feature in NodeJS going forward (again, I would hate to rely on this if it's likely to be reversed in v7 or v8).

@oliversalzburg
Copy link
Contributor Author

@nfischer The code relies on the GetEnvironmentVariableW Windows API function, which already has case-insensitive behavior. I would consider it extremely unlikely that this behavior would ever change.

benjamingr pushed a commit that referenced this issue Oct 25, 2016
Environment variables should be treated case-insensitive on Windows
platforms and case-sensitive on UNIX platforms.

This commit ensures this behavior persists.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
evanlucas pushed a commit that referenced this issue Nov 2, 2016
On Windows OS, environment variables are case-insensitive and are
treated likewise in NodeJS. This can be confusing and can lead
to hard-to-debug problems when moving code from one environment
to another.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
evanlucas pushed a commit that referenced this issue Nov 2, 2016
Environment variables should be treated case-insensitive on Windows
platforms and case-sensitive on UNIX platforms.

This commit ensures this behavior persists.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
MylesBorins pushed a commit that referenced this issue Nov 17, 2016
Environment variables should be treated case-insensitive on Windows
platforms and case-sensitive on UNIX platforms.

This commit ensures this behavior persists.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
MylesBorins pushed a commit that referenced this issue Nov 17, 2016
On Windows OS, environment variables are case-insensitive and are
treated likewise in NodeJS. This can be confusing and can lead
to hard-to-debug problems when moving code from one environment
to another.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
MylesBorins pushed a commit that referenced this issue Nov 19, 2016
Environment variables should be treated case-insensitive on Windows
platforms and case-sensitive on UNIX platforms.

This commit ensures this behavior persists.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
MylesBorins pushed a commit that referenced this issue Nov 19, 2016
On Windows OS, environment variables are case-insensitive and are
treated likewise in NodeJS. This can be confusing and can lead
to hard-to-debug problems when moving code from one environment
to another.

Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #9166
Fixes: #9157
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. good first issue Issues that are suitable for first-time contributors. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants