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

Make "process.env" case insensitive in Windows #5465

Merged
merged 1 commit into from
Feb 5, 2018
Merged

Make "process.env" case insensitive in Windows #5465

merged 1 commit into from
Feb 5, 2018

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Feb 5, 2018

When we fully sandboxed process, we removed the ability of the env object to perform case-insensitive matches when used in Windows. The proposed solution uses a Proxy instance over a shared prototype object, and emulates the behavior by using a secondary lookup object.

Another behavior that process.env has is that all values are always stored as strings, regardless of their value (numbers, booleans...). This behavior has also been mimicked in the new implementation.

Tests were also added to ensure that both behaviors are well respected in different platforms. Fixes #5322.

const BLACKLIST = new Set(['mainModule', '_events']);
const BLACKLIST = new Set(['env', 'mainModule', '_events']);

function createProcessEnv() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it will be helpful to point to the process.env docs somewhere here (e.g. I wasn't aware of this, TIL): https://nodejs.org/api/process.html#process_process_env

@thymikee
Copy link
Collaborator

thymikee commented Feb 5, 2018

CI failures are completely unrelated, what the heck CircleCI?

@mjesun
Copy link
Contributor Author

mjesun commented Feb 5, 2018

It's trying to use what it seems to be a corrupted cache layer ("Found a cache from build 13420 at dependencies-pull/5465-ogGKRVLu81JjC2jiSjH1QgvfyezJ8TPaAaAJSg_G0Vk=").

@mjesun
Copy link
Contributor Author

mjesun commented Feb 5, 2018

I'm going to merge this. CircleCI looks corrupted, but tests passed in AppVeyor.

@mjesun mjesun merged commit ae3d55e into jestjs:master Feb 5, 2018
@mjesun mjesun deleted the fix-process-env branch February 5, 2018 14:54
@@ -30,3 +30,47 @@ it('creates a process object that looks like the original one', () => {
it('fakes require("process") so it is equal to "global.process"', () => {
expect(require('process') === process).toBe(true);
});

it('checks that process.env works as expected on Linux platforms', () => {
Copy link
Member

Choose a reason for hiding this comment

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

could you add a test for delete process.env.PROP as well?

@SimenB
Copy link
Member

SimenB commented Feb 5, 2018

Change looks great 🙂

Missing changelog entry 😱

@mjesun
Copy link
Contributor Author

mjesun commented Feb 5, 2018

@SimenB Good catch on the delete one! Maybe @janpieterz, who originally reported the issue can help us know whether deletion is also case-insensitive. Once I know I will implement that too 🙂

@SimenB
Copy link
Member

SimenB commented Feb 5, 2018

@mjesun It is case insensitive:

image

@mjesun
Copy link
Contributor Author

mjesun commented Feb 5, 2018

What about delete process.env.foo? Does it delete FOO?

@SimenB
Copy link
Member

SimenB commented Feb 5, 2018

Yup.

image

@mjesun
Copy link
Contributor Author

mjesun commented Feb 5, 2018

Thanks! I will implement that behavior as well 🙂

@cpojer
Copy link
Member

cpojer commented Feb 5, 2018

And that is the story of how we started using Proxies in production… :D

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running tests with jest seems to trigger a change in environment variables
5 participants