Fix node-tests package to propagate errors
#276
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 2. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes error propagation in the node-tests package by preventing Metro's guardedLoadModule from swallowing errors during module initialization. The solution defers module initialization from require-time to test execution time.
Key changes:
- Modified test entrypoint generation to export test functions directly instead of wrapping them in arrow functions
- Added a Rolldown plugin to replace the default export pattern, converting immediate function invocation to function references
- Added a convenience npm script for running Mocha with Metro
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/node-tests/scripts/generate-entrypoint.mts | Removes arrow function wrapper from test suite generation to defer module initialization |
| packages/node-tests/rolldown.config.mts | Adds replace plugin to convert require_test() invocations to require_test references, deferring execution |
| apps/test-app/package.json | Adds mocha-and-metro npm script for convenient test execution |
| // Replace the default export to return a function instead of initializing the addon immediately | ||
| // This allows the test runner to intercept any errors which would normally be thrown when importing | ||
| // to work around Metro's `guardedLoadModule` swallowing errors during module initialization | ||
| // See https://github.com/facebook/metro/blob/34bb8913ec4b5b02690b39d2246599faf094f721/packages/metro-runtime/src/polyfills/require.js#L348-L353 |
There was a problem hiding this comment.
[nitpick] The comment references a specific GitHub commit hash in the Metro repository that may become outdated. Consider adding a version number or date reference (e.g., 'Metro v0.80.x as of Oct 2024') alongside the commit hash to provide more context about when this workaround was needed.
| // See https://github.com/facebook/metro/blob/34bb8913ec4b5b02690b39d2246599faf094f721/packages/metro-runtime/src/polyfills/require.js#L348-L353 | |
| // See https://github.com/facebook/metro/blob/34bb8913ec4b5b02690b39d2246599faf094f721/packages/metro-runtime/src/polyfills/require.js#L348-L353 | |
| // (Metro v0.80.x as of Oct 2024) |
kraenhansen
force-pushed
the
kh/node-tests-fix
branch
from
e12f315 to
5c24761
Compare
The metro require function was catching and reporting uncaught errors instead of propagating them to the caller of
require. This was messing up outnode-tests.Merging this PR will: