lib: align console.table row to the left by MrJithil · Pull Request #50135 · nodejs/node · GitHub

MDN: https://developer.mozilla.org/en-US/docs/Web/API/console/table

I am fine with this, but I checked the "spec" and it says nothing regarding the output. Its even worse, LOL

See

https://console.spec.whatwg.org/#table

I am fine with this, but I checked the "spec" and it says nothing regarding the output. Its even worse, LOL

See

https://console.spec.whatwg.org/#table

Yeah. The WHATWG, is not suppose mention the standards in very detail. They have the links to MDN.

Yes and the mdn does not state that the entries are left aligned but just contains a screenshot.

What was the original reason for centering it in Node.js? Is there a git blame?

I have the feeling this might break some stuff based on snapshots/benchmarks

LGTM this aligns with browser behavior for a primarily browser-compat-reasons API.

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

LGTM

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

Landed in 14af167

It hasn't released yet. Thanks for catching up @marco-ippolito.

Update: Wait, if this is now following the SPEC I wonder if this should be considered semver-major.

The spec doesn't say anything about alignment, technically Node.js was spec-compliant before and after this PR landed. What changes is now Node.js output is closer to what browsers DevTools output.

But anyway, I don't really how this change could break anyone, AFAICT the change is only visual.

AFAICT the change is only visual.

Output changes are considered semver-major. Marco tagged it because we've just mentioned it during the collab summit.

It hasn't released yet. Thanks for catching up @marco-ippolito.

Update: Wait, if this is now following the SPEC I wonder if this should be considered semver-major.

There is no SPEC, now it just has the same behavior of browser

AFAICT the change is only visual.

Output changes are considered semver-major. Marco tagged it because we've just mentioned it during the collab summit.

I don’t think that’s true, for example changing an error message technically changes the output, yet we don’t consider it semver-major. Also the output of console.table is not observable from JavaScript without watching directly the stdout.

I don’t think that’s true, for example changing an error message technically changes the output, yet we don’t consider it semver-major

We do consider error message changes as semver-major or at least, we should.

@RafaelGSS that's not what we say in our documentation:

node/doc/api/errors.md

Lines 285 to 289 in 7c1b1f4

    
           The `error.code` property is a string label that identifies the kind of error. 
        
           `error.code` is the most stable way to identify an error. It will only change 
        
           between major versions of Node.js. In contrast, `error.message` strings may 
        
           change between any versions of Node.js. See [Node.js error codes][] for details 
        
           about specific codes.

We also recommend testing only the error code, not the message:

node/doc/contributing/writing-tests.md

Lines 315 to 325 in 260092e

    
           In the case of internal errors, prefer checking only the `code` property: 
        
           ```js 
        
           assert.throws( 
        
             () => { 
        
               throw new ERR_FS_FILE_TOO_LARGE(`${sizeKiB} Kb`); 
        
             }, 
        
             { code: 'ERR_FS_FILE_TOO_LARGE' }, 
        
             // Do not include message: /^File size ([0-9]+ Kb) is greater than 2 GiB$/ 
        
           ); 
        
           ```

AFAICT, it's always been the case that we have been treating error message update as semver-patch, and recommend users to use the code property to detect errors.

Anyway, to come back to this PR, I really think this is not semver-major, but it's not a hill I'll die on.

Oh, I haven't seen or I don't remember these sections, thanks for pointing that out. If we have a doc or a precedence, I won't object to it. @marco-ippolito I will remove the label. In case you strong believe it should be semver-major, ping me.

FWIW a resource I found in a quick search #3374.

UPDATE: It was discussed when error.code didn't exist at the time (that is what I understood from this discussion: https://github.com/nodejs/node/pull/4664/files#diff-bbed55d3167db7c16baf3ee2c51acb1b09f7740ab1399ab015226c3df488753aR46). So it's not a fair discussion.

The reason why we have this entire error.code system (and a former strategic initiative involving years of work assigning these codes) was to prevent having to treat error.message changes as server major. Only errors that are known to have their messages captured in the user land (mostly system errors) can still get the semver major treatment but otherwise error message changes are considered semver patch.

I believe @mcollina mentioned output changes in console were semver major.
I'm not sure the comparison it's fair between error.message and console.table,
console.table is widely used for benchmarks and snapshots, in documentation etc..., the output actually matters.
but I'm not gonna insist if it's something documented

output changes can breaking. Maybe let's await backporting to LTS for a bit?

PR-URL: #50135 Fixes: #50117 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

PR-URL: #50553 Refs: #50135 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>

PR-URL: #50135 Fixes: #50117 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

PR-URL: #50553 Refs: #50135 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>

This should likely not have been backported to v20, it broke our tests.
It also broke Undici tests.

This should likely not have been backported to v20, it broke our tests.
It also broke Undici tests.

Can we clearly add to the documentation that console output changes should be semver major?

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 11, 2023

meyfa reviewed Oct 11, 2023

View reviewed changes

Comment thread lib/internal/cli_table.js Outdated

MrJithil force-pushed the fix-50117-console-table-alignment branch from 6bbf779 to 2684d81 Compare October 11, 2023 14:38

benjamingr approved these changes Oct 22, 2023

View reviewed changes

lib: align console.table row to the left

92ad82b

MrJithil force-pushed the fix-50117-console-table-alignment branch from 2684d81 to 92ad82b Compare October 25, 2023 13:08

H4ad added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023

Uzlopak approved these changes Oct 25, 2023

View reviewed changes

This was referenced Oct 26, 2023

CI Reliability 2023-10-26 nodejs/reliability#698

Open

CI Reliability 2023-10-27 nodejs/reliability#699

Open

trivikr approved these changes Oct 27, 2023

View reviewed changes

github-actions Bot mentioned this pull request Oct 28, 2023

CI Reliability 2023-10-28 nodejs/reliability#700

Open

22 tasks

H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 28, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 28, 2023

nodejs-github-bot merged commit 14af167 into nodejs:main Oct 28, 2023

This was referenced Oct 29, 2023

CI Reliability 2023-10-29 nodejs/reliability#701

Open

CI Reliability 2023-10-30 nodejs/reliability#702

Open

CI Reliability 2023-10-31 nodejs/reliability#703

Open

pluris mentioned this pull request Nov 4, 2023

doc: update to align console.table row to the left #50553

Merged

RafaelGSS removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 11, 2023

MrJithil deleted the fix-50117-console-table-alignment branch November 11, 2023 11:20

MrJithil restored the fix-50117-console-table-alignment branch November 11, 2023 11:20

MrJithil deleted the fix-50117-console-table-alignment branch November 11, 2023 11:20

MrJithil restored the fix-50117-console-table-alignment branch November 11, 2023 11:20

targos pushed a commit that referenced this pull request Nov 11, 2023

lib: align console.table row to the left

3615a61

PR-URL: #50135 Fixes: #50117 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

targos mentioned this pull request Nov 12, 2023

v21.2.0 release proposal #50681

Merged

UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023

lib: align console.table row to the left

e7551d5

PR-URL: #50135 Fixes: #50117 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

UlisesGascon mentioned this pull request Dec 12, 2023

v20.11.0 proposal #51124

Merged

sosukesuzuki mentioned this pull request Dec 31, 2023

chore: update pkgjs/action from 0.1.7 to 0.1.9 nodejs/undici#2567

Closed

5 tasks

sosukesuzuki mentioned this pull request Jan 11, 2024

Fix tests for Node.js v21 nodejs/undici#2609

Merged

7 tasks

marco-ippolito mentioned this pull request Jan 17, 2024

doc: document console changes as breaking #51503

Closed

lib: align console.table row to the left by MrJithil · Pull Request #50135 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: align console.table row to the left - #50135

lib: align console.table row to the left#50135nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom MrJithil:fix-50117-console-table-alignmentMrJithil/node:fix-50117-console-table-alignmentCopy head branch name to clipboard

Conversation

MrJithil commented Oct 11, 2023 • edited by RaisinTen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uzlopak commented Oct 11, 2023

Uh oh!

MrJithil commented Oct 11, 2023

Uh oh!

Uzlopak commented Oct 11, 2023

Uh oh!

meyfa commented Oct 11, 2023

Uh oh!

Uh oh!

marco-ippolito commented Oct 22, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 27, 2023

Uh oh!

nodejs-github-bot commented Oct 28, 2023

Uh oh!

RafaelGSS commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RafaelGSS commented Nov 8, 2023

Uh oh!

marco-ippolito commented Nov 8, 2023

Uh oh!

aduh95 commented Nov 8, 2023

Uh oh!

RafaelGSS commented Nov 10, 2023

Uh oh!

aduh95 commented Nov 11, 2023

Uh oh!

RafaelGSS commented Nov 11, 2023

Uh oh!

RafaelGSS commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marco-ippolito commented Nov 11, 2023

Uh oh!

mcollina commented Nov 11, 2023

Uh oh!

mcollina commented Jan 17, 2024

Uh oh!

marco-ippolito commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

lib: align console.table row to the left#50135
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MrJithil:fix-50117-console-table-alignment

MrJithil commented Oct 11, 2023 •
edited by RaisinTen
Loading

RafaelGSS commented Nov 8, 2023 •
edited
Loading

aduh95 commented Nov 8, 2023 •
edited
Loading

RafaelGSS commented Nov 11, 2023 •
edited
Loading

joyeecheung commented Nov 11, 2023 •
edited
Loading