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.

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

Lenient stackTraceLimit works across browsers #591

Merged
merged 3 commits into from
Mar 1, 2021

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 1, 2021

Fixes #525

Using #590 to iterate within several browsers, I repaired several issues.

  • Fix Error.stackTraceLimit assignment on Firefox #525 was that assigning Error.stackTraceLimit was failing in non-v8-based browsers because Error had no such property and is frozen. Although the property is indeed non-standard and absent in non-v8 systems, Google's documentation encourages assignments to it anyway. This is a noop elsewhere, but is harmless when Error is unfrozen. To accommodate this practice, this PR installs a noop Error.stackTraceLimit accessor property on every platform. It is a noop everywhere except in the start compartment of a v8-based platform.
  • On the Safari browser (and presumably on any host using the JSC engine) DataView has a non-standard but non-deletable BYTES_PER_ELEMENT property whose value is a number. Since it is non-deletable but harmless, we added it to our whitelist.
  • We added logic for obtaining the stack string for FF (using the proposed Error.prototype.stack accessor property) and others including Safari using the error.stack` property if present. Otherwise (such as on XS currently) the default stack remains the empty string.

In the process, made the console output more pleasant for the common case of console messages containing only one error argument. For this case, nothing is gained by nesting the error within a group. Removing this extra visual noise helps this common case.

Because I built it using #590 it was easy to build it on #590 , which is fine if #590 gets merged first. If #590 gets delayed, I can make this independently mergeable since it has no logical dependence on #590.

Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

At a glance, I don't see anything wrong, but I'm not confident I could maintain this, so I'm inclined to leave it to others.

Copy link
Contributor

@warner warner left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, but I'll defer to @michaelfig or @kriskowal as the real experts.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM.

packages/ses/src/error/console.js Outdated Show resolved Hide resolved
Base automatically changed from markm-repair-demo-console to master March 1, 2021 22:48
@erights erights force-pushed the 525-markm-lenient-stackTraceLimit branch 2 times, most recently from 73c1c04 to aaf25fa Compare March 1, 2021 22:52
@erights erights force-pushed the 525-markm-lenient-stackTraceLimit branch from aaf25fa to 3eb49fd Compare March 1, 2021 22:53
@erights erights enabled auto-merge (squash) March 1, 2021 23:10
@erights erights merged commit 299abe0 into master Mar 1, 2021
@erights erights deleted the 525-markm-lenient-stackTraceLimit branch March 1, 2021 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Error.stackTraceLimit assignment on Firefox
5 participants