Skip to content

util: inspect: enumerable Symbols no longer have square brackets#55778

Closed
ljharb wants to merge 2 commits intonodejs:mainfrom
ljharb:nonenums
Closed

util: inspect: enumerable Symbols no longer have square brackets#55778
ljharb wants to merge 2 commits intonodejs:mainfrom
ljharb:nonenums

Conversation

@ljharb
Copy link
Member

@ljharb ljharb commented Nov 8, 2024 •
edited
Loading

The intention of this change is to increase consistency. Before this PR, non-enumerable string properties have square brackets, and all Symbol properties have square brackets - meaning that without color cues, there's no way to determine whether a Symbol is enumerable or not, and it can be confusing whether [foo] is indeed bracketed because it's non-enumerable.

Due to the following considerations:

  1. minimizing disruption/changes to output
  2. square brackets are already used for a number of "special" things, including a data proto property, Symbol.toStringTag values, null objects, etc, so we already wouldn't be able to have square brackets mean only "computed properties"

… we decided to go with "enumerable Symbols do not have square brackets", so that all non-enumerables use square brackets.

Implements https://github.com/orgs/nodejs/discussions/41283#discussioncomment-11188239

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Nov 8, 2024
@ljharb
Copy link
Member Author

ljharb commented Nov 8, 2024

cc @BridgeAR

@avivkeller
Copy link
Member

avivkeller commented Nov 8, 2024 •
edited
Loading

IMO this is a semver-major breaking change, as it breaks the existing behavior of brackets existing on all symbols

BridgeAR
BridgeAR reviewed Nov 8, 2024
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Please also add a few tests for the now escaped non-enumerable key names that need escaping.

I am fine to consider it as semver-major, while we normally do not handle these changes as being part of semver. The output changes from time to time and it is considered a debugging utility that does not have a stable output. I suggest to just add a few do not land labels for now and consider it a semver minor change? It could theoretically be back parted at a later point that way.

@avivkeller avivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 8, 2024
@BridgeAR
Copy link
Member

BridgeAR commented Nov 8, 2024

@ljharb I would probably just take the gist out of the discussion and use that as commit description. That way it's clear that this is done for consistency reasons to distinguish non-enumerable properties easier.

@avivkeller
Copy link
Member

avivkeller commented Nov 8, 2024

@BridgeAR I've added semver-major PRs that contain breaking changes and should be released in the next major version. for now, but if something changes, we can always adjust it to your suggestion.

@targos targos added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Nov 8, 2024
@codecov
Copy link

codecov bot commented Nov 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (58a8eb4) to head (577b7e7).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55778      +/-   ##
==========================================
- Coverage   88.40%   88.40%   -0.01%     
==========================================
  Files         654      654              
  Lines      187815   187810       -5     
  Branches    36136    36140       +4     
==========================================
- Hits       166045   166035      -10     
- Misses      15001    15014      +13     
+ Partials     6769     6761       -8
Files with missing lines Coverage Δ
lib/internal/util/inspect.js 99.95% <100.00%> (-0.05%) ⬇️

... and 37 files with indirect coverage changes

@ljharb ljharb force-pushed the nonenums branch 2 times, most recently from 8a7eda3 to 02d01fb Compare November 8, 2024 13:54
@ljharb ljharb force-pushed the nonenums branch 5 times, most recently from 577b7e7 to f4a5af6 Compare November 10, 2024 20:21
@ljharb

This comment was marked as resolved.

Sign in to view
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 11, 2024

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

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 12, 2024

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

@ljharb
Copy link
Member Author

ljharb commented Nov 12, 2024

Of the 2 failures, both appear to be unrelated to the PR, and seem to be issues with git itself. Can someone take a look?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 12, 2024

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

@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 13, 2024
@ljharb
Copy link
Member Author

ljharb commented Nov 14, 2024

CI doesn't seem to be running, despite someone resuming it multiple times. Can someone look into it?

@RafaelGSS
Copy link
Member

RafaelGSS commented Nov 14, 2024

It seems nodejs/build#3959

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 14, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 15, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 15, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 16, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 16, 2024

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

@ljharb
Copy link
Member Author

ljharb commented Nov 17, 2024

yay, all green!

@atlowChemi atlowChemi added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Nov 17, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 17, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 17, 2024

Landed in 9f2885a...e577618

nodejs-github-bot pushed a commit that referenced this pull request Nov 17, 2024
@ljharb @nodejs-github-bot
test: add test case for util.inspect
23275cc 
PR-URL: #55778
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 17, 2024
@ljharb @nodejs-github-bot
util: inspect: enumerable Symbols no longer have square brackets
e577618 
Implements https://github.com/orgs/nodejs/discussions/41283#discussioncomment-11188239

PR-URL: #55778
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
@ljharb ljharb deleted the nonenums branch November 17, 2024 15:08
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@ljharb @tpoisseau
test: add test case for util.inspect
96d9e05 
PR-URL: nodejs#55778
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@ljharb @tpoisseau
util: inspect: enumerable Symbols no longer have square brackets
2f619ff 
Implements https://github.com/orgs/nodejs/discussions/41283#discussioncomment-11188239

PR-URL: nodejs#55778
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
@ljharb @Ceres6
test: add test case for util.inspect
575f1f5 
PR-URL: nodejs#55778
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
@ljharb @Ceres6
util: inspect: enumerable Symbols no longer have square brackets
a7d08cd 
Implements https://github.com/orgs/nodejs/discussions/41283#discussioncomment-11188239

PR-URL: nodejs#55778
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MoLow MoLow MoLow approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@juanarbol juanarbol juanarbol approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@ljharb @avivkeller @BridgeAR @nodejs-github-bot @RafaelGSS @MoLow @juanarbol @atlowChemi @targos @marco-ippolito