Skip to content

src: handle indexed properties in process.env#60826

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Nov 27, 2025
Merged

src: handle indexed properties in process.env#60826

nodejs-github-bot merged 1 commit into from
Nov 27, 2025
+58 −0

Conversation

@targos
Copy link
Member

@targostargos commented Nov 23, 2025

Closes: #60795

I'm not sure if we should treat this as a breaking change.
Before this change, indexed properties would not be intercepted, so setting them in JS would not coerce the value to a string.

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 23, 2025
@targostargos added the process Issues and PRs related to the process subsystem. label Nov 23, 2025
@targostargos mentioned this pull request Nov 23, 2025
Closed
@codecov
Copy link

codecovbot commented Nov 23, 2025•
edited
Loading

Codecov Report

❌ Patch coverage is 70.37037% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (430db0d) to head (ef1b42c).
⚠️ Report is 8 commits behind head on main.

Files with missing linesPatch %Lines
src/node_env_var.cc70.37%8 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #60826 +/- ## ========================================== + Coverage 88.55% 88.56% +0.01%  ========================================== Files 703 703 Lines 208237 208264 +27 Branches 40157 40162 +5 ========================================== + Hits 184395 184448 +53 + Misses 15858 15827 -31 - Partials 7984 7989 +5
Files with missing linesCoverage Δ
src/node_env_var.cc82.21% <70.37%> (-0.93%)⬇️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Renegade334
Copy link
Member

Renegade334 commented Nov 23, 2025

Are there consequences from having the named property enumerator emit numeric values? (I know that's what happens currently...)

@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 23, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 23, 2025

CI: https://ci.nodejs.org/job/node-test-pull-request/70294/

legendecas
legendecas reviewed Nov 24, 2025
View reviewed changes
legendecas
legendecas reviewed Nov 25, 2025
View reviewed changes
@targostargos marked this pull request as draft November 25, 2025 07:00
@aduh95aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 25, 2025
@targostargos marked this pull request as ready for review November 27, 2025 07:33
@targostargos changed the title src: handled indexed properties in process.envsrc: handle indexed properties in process.envNov 27, 2025
@targos
Copy link
MemberAuthor

targos commented Nov 27, 2025

Rebased to include #60846 and fixed a typo in the commit message.

@targos
Copy link
MemberAuthor

targos commented Nov 27, 2025

Are there consequences from having the named property enumerator emit numeric values? (I know that's what happens currently...)

@Renegade334 I think it only affects users of the C++ API, which allows to skip indices.

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 27, 2025

CI: https://ci.nodejs.org/job/node-test-pull-request/70328/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 27, 2025

CI: https://ci.nodejs.org/job/node-test-pull-request/70330/

@targostargos added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2025
@nodejs-github-botnodejs-github-bot merged commit 08d966c into nodejs:mainNov 27, 2025
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 27, 2025

Landed in 08d966c

@targostargos deleted the process-env-indexed branch November 27, 2025 13:17
targos added a commit that referenced this pull request Nov 29, 2025
@targos
src: handle indexed properties in process.env
1ce2fb9
Closes: #60795 PR-URL: #60826Fixes: #60795 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
aduh95 pushed a commit that referenced this pull request Jan 9, 2026
@targos@aduh95
src: handle indexed properties in process.env
a8fcc0b
Closes: #60795 PR-URL: #60826Fixes: #60795 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@cjihrigcjihrigcjihrig approved these changes

@legendecaslegendecaslegendecas approved these changes

@StefanStojanovicStefanStojanovicStefanStojanovic approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.processIssues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

process.env is missing environment variables with numeric names

8 participants

@targos@Renegade334@nodejs-github-bot@addaleax@cjihrig@legendecas@StefanStojanovic@aduh95