-
Notifications
You must be signed in to change notification settings - Fork 43
refactor: Make executeSafePromise work in browsers without polyfills
#3310
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
Conversation
Otherwise `process` breaks in browsers.
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.
Pull request overview
This PR refactors the executeSafePromise utility to improve cross-platform compatibility, specifically making it work in browser environments without requiring polyfills. The key improvements focus on robust environment detection and proper error handling for both Node/Electron and browser contexts.
Key changes:
- Enhanced environment detection by checking if
processis defined before attempting to callprocess.exit() - Updated browser error handling to use the
causeproperty for proper error chaining, following modern JavaScript standards - Improved documentation to clarify behavior differences between Node/Electron and browser environments
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mondoreale I've opened a new pull request, #3311, to work on those changes. Once the pull request is ready, I'll request review from you. |
…#3311) ## Summary Adds missing test coverage for error handling paths in `executeSafePromise` introduced by the browser compatibility refactor (#3310). ## Changes - **Node/Electron environment tests**: Verify `process.exit(1)` is called on promise rejection and fatal errors are logged - **Browser environment tests**: Verify errors are thrown with proper error chaining via `cause` property The environment detection behavior is implicitly verified by the Node and browser test blocks, which mock or remove `process.exit` and verify the appropriate code path is taken. ## Checklist before requesting a review - [ ] Is this a breaking change? If it is, be clear in summary. - [x] Read through code myself one more time. - [x] Make sure any and all `TODO` comments left behind are meant to be left in. - [x] Has reasonable passing test coverage? - [ ] Updated changelog if applicable. - [ ] Updated documentation if applicable. <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/streamr-dev/network/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Expands test coverage for `executeSafePromise` to validate error-handling in different runtimes. > > - Adds Node/Electron tests in `packages/utils/test/executeSafePromise.test.ts` to assert `process.exit(1)` is invoked on rejection and that exit is triggered after fatal errors > - Adds browser-environment tests by removing `process.exit` to assert a thrown error with message `executeSafePromise: Assertion failure!` and proper `cause` chaining > - Retains success-path test to confirm resolved value handling > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit ff0e81f. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mondoreale <[email protected]>
|
|
This reverts commit 8af348f.
This pull request updates the
executeSafePromiseutility to improve its error handling across different environments. The main enhancement is more robust detection of whether the code is running in Node/Electron or in a browser, and handling fatal promise rejections appropriately in each case.Changes
Improvements to error handling and environment detection:
Updated the documentation for
executeSafePromiseto clarify that in Node/Electron, the process exits on fatal errors, while in browsers, an unhandled error is thrown.Improved environment detection by explicitly checking if
processis defined before callingprocess.exit, ensuring compatibility with browser environments.In browser environments, changed the thrown error to use the
causeproperty for proper error chaining, aligning with modern JavaScript error handling best practices.Future steps
We may want to improve test coverage on this. I've tried but karma doesn't let it slide so easily.