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.

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

Change console.assert summary to match API summary #30725

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Change console.assert summary to match API summary #30725

merged 2 commits into from
Mar 27, 2024

Conversation

DanKaplanSES
Copy link
Contributor

Description

Like console.log(errorObj), console.assert(condition, errorObj) prints a stack trace. But console log's summary doesn't mention this detail, nor does the API for console.assert. Therefore, it felt more consistent to remove it here.

Motivation

See above

Additional details

Modified page: https://developer.mozilla.org/en-US/docs/Web/API/console#instance_methods
console.assert page: https://developer.mozilla.org/en-US/docs/Web/API/console/assert_static
console.log page: https://developer.mozilla.org/en-US/docs/Web/API/console/log_static

Related issues and pull requests

@DanKaplanSES
Change console.assert summary to match API summary
7c8d116 
- Like `console.log(errorObj)`, `console.assert(condition, errorObj)` prints a stack trace. But `console log`'s summary doesn't mention this detail, nor does the API for `console.assert`. Therefore, it felt more consistent to remove it here.
@DanKaplanSES DanKaplanSES requested a review from a team as a code owner December 1, 2023 23:09
@DanKaplanSES DanKaplanSES requested review from wbamberg and removed request for a team December 1, 2023 23:09
@github-actions github-actions bot added the Content:WebAPI Web API docs label Dec 1, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 1, 2023 •
edited
Loading

Preview URLs

(comment last updated: 2024-02-27 03:29:42)

Josh-Cena
Josh-Cena reviewed Dec 1, 2023
View reviewed changes
Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

The stack (in browsers) is printed by assert itself. However in Node console.assert doesn't print the stack.

@DanKaplanSES
Copy link
Contributor Author

The stack (in browsers) is printed by assert itself.

Thank you for pointing that out. To clarify, you are referring to when an Error object is passed in? Can you let me know if there's anything specific I should consider changing based on this information? Thanks again.

@Josh-Cena
Copy link
Member

I'm not sure. At least consider the following:

image

No Error instances, still have stack trace.

@DanKaplanSES
Copy link
Contributor Author

DanKaplanSES commented Dec 2, 2023 •
edited
Loading

I'm not sure. At least consider the following:
No Error instances, still have stack trace.

Did you do that in a Firefox devtool console? I tried the same code and it didn't print a stack trace:

image

Do I have a typo?

function f() {
	function g() {
		console.assert(false, "foo");
	}
	g();
}
f();

Before my reply I open this local file and it had similar output:

<html>
  <body>
    <script>
      (function () {console.assert(false, "failure"); console.log("hi");})()
    </script>
  </body>
</html>

image

I am using the latest version of Firefox.

@Josh-Cena
Copy link
Member

Ah yes, mine was Chrome... I guess it's not consistent across browsers either 😄

@DanKaplanSES
Copy link
Contributor Author

Ah yes, mine was Chrome... I guess it's not consistent across browsers either 😄

Interesting... Would that go as a note in the compatibility table?

@Josh-Cena
Copy link
Member

I don't know for sure. The console spec is messy and nothing is known for certain. I'll let others decide.

@wbamberg wbamberg removed their request for review December 4, 2023 19:45
@github-actions github-actions bot added the size/xs [PR only] 0-5 LoC changed label Feb 27, 2024
@wbamberg
Copy link
Collaborator

FWIW I think we should accept this PR whether or not we decide to update the BCD.

I know, console support is super-messy, but if we ever did want to address cross-platform divergences (rather than saying as we currently do that "The specifics of how it works vary from browser to browser or server runtimes, but there is a de facto set of features that are typically provided.") then that would be a separate thing from this PR.

@bsmth
Copy link
Member

bsmth commented Mar 26, 2024

I'm just adding a +1 to what Will says, this LGTM for now and clearing up implementation docs is a bigger task. I'm not sure if BCD is the right place for it (maybe it is) but it would be nice to have a programmatic way to test and display this.

@wbamberg
Copy link
Collaborator

@Josh-Cena , are you OK to merge this, based on #30725 (comment)?

@Josh-Cena
Copy link
Member

Yes, I have no opinions either way.

bsmth
bsmth approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thanks, all - going ahead and merging 👍🏻

@bsmth bsmth merged commit 2ceee44 into mdn:main Mar 27, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Josh-Cena Josh-Cena Josh-Cena left review comments

@bsmth bsmth bsmth approved these changes

Assignees
No one assigned
Labels
Content:WebAPI Web API docs size/xs [PR only] 0-5 LoC changed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DanKaplanSES @Josh-Cena @wbamberg @bsmth