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

feat: implement a RequestController class #595

Merged
merged 19 commits into from
Jul 11, 2024
Merged

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Jul 4, 2024

Warning

This is a breaking change. The way you handle requests changes from request.respondWith() to controller.respondWith().

This change also lays foundation for supporting controller.abort() (or even better—use AbortSignal) to abort any intercepted request. This would be awesome to ship in the future.

Roadmap

  • Refactor existing utils/RequestController.ts class. It's kind of similar but is used for a deferred response resolution. I suspect we can throw it away.
  • Remove request.respondWith().
  • (Probably) Drop the concept of "interactive request".
  • Add controller: RequestController to the request event listener arguments.
  • Check one suspicious test
  • Add unit tests for RequestController
  • ClientRequest: drop the respondWith multiple calls tests from these unit tests
  • RemoteInterceptor: implement the serialization contract to support the onResponseError and onAborted scenarios (these were not supported previously and can be treated as a separate bugfix)

@kettanaito
Copy link
Member Author

kettanaito commented Jul 5, 2024

Suspicious test

stderr | modules/XMLHttpRequest/compliance/xhr-middleware-exception.test.ts > handles exceptions as instructed in "unhandledException" listener (request error)
Error: Error: connect ECONNREFUSED 127.0.0.1:80
    at Object.dispatchError (/Users/kettanaito/Projects/mswjs/interceptors/node_modules/.pnpm/[email protected]/node_modules/jsdom/lib/jsdom/living/xhr/xhr-utils.js:63:19)
    at EventEmitter.<anonymous> (/Users/kettanaito/Projects/mswjs/interceptors/node_modules/.pnpm/[email protected]/node_modules/jsdom/lib/jsdom/living/xhr/XMLHttpRequest-impl.js:655:18)
    at EventEmitter.emit (node:events:525:35)
    at Request.<anonymous> (/Users/kettanaito/Projects/mswjs/interceptors/node_modules/.pnpm/[email protected]/node_modules/jsdom/lib/jsdom/living/xhr/xhr-utils.js:401:14)
    at Request.emit (node:events:513:28)
    at ClientRequest.<anonymous> (/Users/kettanaito/Projects/mswjs/interceptors/node_modules/.pnpm/[email protected]/node_modules/jsdom/lib/jsdom/living/helpers/http-request.js:121:14)
    at ClientRequest.emit (node:events:513:28)
    at Socket.socketErrorListener (node:_http_client:502:9)
    at Socket.emit (node:events:513:28)
    at emitErrorNT (node:internal/streams/destroy:151:8) undefined

   · sets "credentials" to "omit" on isomorphic request when "withCredentials" is not set
   · sets "credentials" to "omit" on isomorphic request when "withCredentials" is false
   · responds with an ArrayBuffer when "responseType" equals "arraybuffer"
Cancelling test run. Press CTRL+c again to exit forcefully.

Not sure if this is intended. The test passes but the error looks weird. Need to check if it's intentional, and fix it if it's not.

Edit: I confirm this doesn't happen on main. Must be something I broke with the unhandled exception handling and what the controller does.

Solution

I incorrectly treated unhandled errors during request handling as aborted requests. While that may be so, that doesn't translate to request aborts as of now (we don't support aborting the request via controller).

The fix was to call errorWith() on the XHR controller when onError is called on the request handler function. I've renamed the function from onAborted to onError so it's more clear when it's called.

@kettanaito kettanaito changed the title feat: implement RequestController class feat: implement a RequestController class Jul 5, 2024
@@ -174,7 +174,7 @@
"@open-draft/logger": "^0.3.0",
"@open-draft/until": "^2.0.0",
"is-node-process": "^1.2.0",
"outvariant": "^1.2.1",
"outvariant": "^1.4.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

outvariant had issues with polymorphic errors and multiple positionals. Fixed it and updated the dependency.

@kettanaito kettanaito marked this pull request as ready for review July 11, 2024 17:00
@kettanaito kettanaito merged commit 73dd07a into main Jul 11, 2024
2 checks passed
@kettanaito kettanaito deleted the feat/request-controller branch July 11, 2024 17:00
@kettanaito
Copy link
Member Author

Released: v0.33.0 🎉

This has been released in v0.33.0!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstract request emitting logic Replace "request.respondWith()" with a Controller
1 participant