-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
lib: align console.table row to the left #50135
lib: align console.table row to the left #50135
Conversation
I am fine with this, but I checked the "spec" and it says nothing regarding the output. Its even worse, LOL See |
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? |
6bbf779
to
2684d81
Compare
I have the feeling this might break some stuff based on snapshots/benchmarks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM this aligns with browser behavior for a primarily browser-compat-reasons API.
2684d81
to
92ad82b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in 14af167 |
PR-URL: nodejs#50135 Fixes: nodejs#50117 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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. |
Output changes are considered |
There is no SPEC, now it just has the same behavior of browser |
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 |
PR-URL: nodejs#50553 Refs: nodejs#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>
We do consider error message changes as |
@RafaelGSS that's not what we say in our documentation: Lines 285 to 289 in 7c1b1f4
We also recommend testing only the error code, not the message: node/doc/contributing/writing-tests.md Lines 315 to 325 in 260092e
AFAICT, it's always been the case that we have been treating error message update as semver-patch, and recommend users to use the 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 |
FWIW a resource I found in a quick search #3374. UPDATE: It was discussed when |
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. |
output changes can breaking. Maybe let's await backporting to LTS for a bit? |
This should likely not have been backported to v20, it broke our tests. |
Can we clearly add to the documentation that console output changes should be semver major? |
Fixes: #50117
MDN: https://developer.mozilla.org/en-US/docs/Web/API/console/table