Skip to content

Conversation

@francislavoie
Copy link

@francislavoie francislavoie commented Dec 9, 2025

Fixes #367

  • Enables request abortion using AbortSignal in all transports. Ensures requests can be cancelled, prevents memory leaks and race conditions, by rejecting requests that are no longer relevant.
  • Uses an AbortError to differentiate RPC errors from aborts.
  • Ran npm i baseline-browser-mapping@latest -D to update browser mappings (fixes warnings when running tests)
  • Ran npm audit fix to fix 3 vulnerabilities (1 low, 1 moderate, 1 high).
  • Improved test coverage to 100%; in addition to abort tests, also adds a test for when fetcher is missing, and a test to cover index.ts exports.

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (4312a28) to head (0bf521e).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #368      +/-   ##
===========================================
+ Coverage   99.75%   100.00%   +0.24%     
===========================================
  Files          11        11              
  Lines         404       430      +26     
  Branches       56        62       +6     
===========================================
+ Hits          403       430      +27     
+ Misses          1         0       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Enables request abortion using AbortSignal in all transports.
The change ensures requests can be cancelled,
prevents memory leaks and improves user experience by rejecting requests that are no longer relevant.

Implements AbortError class to differentiate RPC errors from aborts.
Ran `npm i baseline-browser-mapping@latest -D` to update browser mappings, and `npm audit fix` to fix 3 vulnerabilities (1 low, 1 moderate, 1 high).
@zcstarr
Copy link
Member

zcstarr commented Dec 9, 2025

Thanks for the PR !!

Going to leave this here, early haven't fully reviewed the PR, but I'd say a few off the top of my head comments.

  • If we can minimize the amount of changes to just be the Error signal so it's easier to merge in and account for.
  • We migrated to bun recently, and so packages should be installed with bun ( I think there's a ci issue that keeps adding back a package-lock.json
  • Is browser based mapping necessary, I think try bun test , if still warning we can always put up a separate PR to fix it ?
  • I think maybe the fetcher part is bun/build related, as bun bundles as well and that might be causing some issues.

I'll comment further when I get a chance to go over it more in depth hopefully sometime this or early next week

@francislavoie
Copy link
Author

francislavoie commented Dec 9, 2025

bun test doesn't pass, fails on all WS and iframe tests

12 tests failed:
✗ WebSocketTransport > can connect [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can send and receive data [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can send and receive data against potential timeout [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can send and receive errors [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can handle underlying transport crash [5000.03ms]
  ^ this test timed out after 5000ms.
✗ PostMessageIframeTransport > iframe > can connect [1.00ms]
✗ PostMessageIframeTransport > iframe > can close
✗ PostMessageIframeTransport > iframe > can send and receive data
✗ PostMessageIframeTransport > iframe > can send and receive data against potential timeout
✗ PostMessageIframeTransport > iframe > can send and receive errors
✗ PostMessageIframeTransport > iframe > can handle underlying transport crash

npm run test passes just fine.

$ npm run test                                                                          

> @open-rpc/[email protected] test
> jest --coverage

 PASS  src/transports/EventEmitterTransport.test.ts
 PASS  src/Error.test.ts
 PASS  src/transports/HTTPTransport.test.ts
 PASS  src/transports/WebSocketTransport.test.ts
 PASS  src/transports/PostMessageIframeTransport.test.ts
[baseline-browser-mapping] The data in this module is over two months old.  To ensure accurate Baseline data, please update: `npm i baseline-browser-mapping@latest -D`
 PASS  src/index.test.ts
 PASS  src/transports/TransportRequestManager.test.ts
 PASS  src/RequestManager.test.ts
 PASS  src/transports/PostMessageWindowTransport.test.ts (23.197 s)
--------------------------------|---------|----------|---------|---------|-------------------
File                            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------------------|---------|----------|---------|---------|-------------------
All files                       |     100 |      100 |     100 |     100 |                   
 src                            |     100 |      100 |     100 |     100 |                   
  Client.ts                     |     100 |      100 |     100 |     100 |                   
  Request.ts                    |     100 |      100 |     100 |     100 |                   
  RequestManager.ts             |     100 |      100 |     100 |     100 |                   
  index.ts                      |     100 |      100 |     100 |     100 |                   
 src/transports                 |     100 |      100 |     100 |     100 |                   
  EventEmitterTransport.ts      |     100 |      100 |     100 |     100 |                   
  HTTPTransport.ts              |     100 |      100 |     100 |     100 |                   
  PostMessageIframeTransport.ts |     100 |      100 |     100 |     100 |                   
  PostMessageWindowTransport.ts |     100 |      100 |     100 |     100 |                   
  Transport.ts                  |     100 |      100 |     100 |     100 |                   
  TransportRequestManager.ts    |     100 |      100 |     100 |     100 |                   
  WebSocketTransport.ts         |     100 |      100 |     100 |     100 |                   
--------------------------------|---------|----------|---------|---------|-------------------

Test Suites: 9 passed, 9 total
Tests:       87 passed, 87 total
Snapshots:   0 total
Time:        24.34 s
Ran all test suites.

Installing baseline is to get rid of that [baseline-browser-mapping] The data in this module is over two months old. To ensure accurate Baseline data, please update: npm i baseline-browser-mapping@latest -D warning of course

If we can minimize the amount of changes to just be the Error signal so it's easier to merge in and account for.

It's just a couple more tests to complete missing coverage, nothing else.

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.

AbortController/AbortSignal

2 participants