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

fix: catch exceptions setting Error.stackTraceLimit #5254

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

kevinoid
Copy link
Contributor

PR Checklist

Overview

When node is run with --frozen-intrinsics, a TypeError is thrown when any intrinsic objects or their properties are modified. This occurs when attempting to set Error.stackTraceLimit. To avoid exiting due to an uncaught exception, catch the exception, debug log it, and continue.

When node is run with [--frozen-intrinsics], a `TypeError` is thrown
when any intrinsic objects or their properties are modified.  This
occurs when attempting to set `Error.stackTraceLimit`.  To avoid exiting
due to an uncaught exception, catch the exception, debug log it, and
continue.

[--frozen-intrinsics]: https://nodejs.org/api/cli.html#--frozen-intrinsics

Signed-off-by: Kevin Locke <[email protected]>
Copy link

linux-foundation-easycla bot commented Nov 14, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@kevinoid
Copy link
Contributor Author

Note: Tests which load unexpected can't currently run with --frozen-intrinsics due to petkaantonov/bluebird#1717 (present in unexpected-bluebird) causing:

✖ ERROR: TypeError <Object <Object <[Object: null prototype] {}>>>: Cannot define property __BluebirdErrorTypes__, object is not extensible
    at Object.defineProperty (<anonymous>)
    at notEnumerableProp (/path/to/mocha/node_modules/unexpected-bluebird/js/main/util.js:106:9)
    at Object.<anonymous> (/path/to/mocha/node_modules/unexpected-bluebird/js/main/errors.js:99:5)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at require (node:internal/modules/helpers:179:18)
    at module.exports (/path/to/mocha/node_modules/unexpected-bluebird/js/main/promise.js:30:14)

Since .mocharc.yml requires test/setup.js which requires unexpected, nearly all invocations of mocha will fail.

We could smoke test this feature by calling node --frozen-intrinsics bin/mocha.js --version or do more thorough testing with a different --config which doesn't require unexpected, but I'm not sure the value is justified by the effort. It may be preferable to wait until the test dependencies support running under --frozen-intrinsics. What do you think?

@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: catch exceptions setting Error.stackTraceLimit fix: catch exceptions setting Error.stackTraceLimit Nov 14, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Nov 14, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@JoshuaKGoldberg
Copy link
Member

This is really more of a bugfix than a feature - so it only necessitates a patch-level version bump.

We're also about to release major version 11 and don't have a separate branch set up to differentiate v10 and v11 releases or support backporting. So this should be good to go only after v11 is released this month.

@JoshuaKGoldberg
Copy link
Member

Quick status update: Mocha 11 is stable and I'm just waiting this week in case something breaks. I'd be surprised if something does, but you never know! #5249

Assuming nothing goes wrong, I plan on merging this early next week. 🙂

@JoshuaKGoldberg
Copy link
Member

Lloyd Bridges as Izzy Mandelbaum in Seinfeld aggressively telling Jerry: "IT'S GO TIME"

@JoshuaKGoldberg
Copy link
Member

Oh, and:

... It may be preferable to wait until the test dependencies support running under --frozen-intrinsics. What do you think?

I think there's no harm in Mocha fixing its support now. Eventually the test dependencies will too. No need for us to be the ones still blocking.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 259f8f8 into mochajs:main Dec 9, 2024
106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants