Skip to content

feat: better errors for Node.JS bindings#406

Merged
barjin merged 10 commits intomasterfrom
feat/node-better-errors
Mar 13, 2026
Merged

feat: better errors for Node.JS bindings#406
barjin merged 10 commits intomasterfrom
feat/node-better-errors

Conversation

@barjin
Copy link
Copy Markdown
Member

@barjin barjin commented Mar 10, 2026

Closes #397.

@barjin barjin self-assigned this Mar 10, 2026
@github-actions github-actions bot added this to the 136th sprint - Tooling team milestone Mar 10, 2026
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Mar 10, 2026
@barjin barjin requested a review from Copilot March 11, 2026 10:25
@barjin barjin marked this pull request as ready for review March 11, 2026 10:29
Copy link
Copy Markdown

Copilot AI left a 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 improves error reporting across the Node.js bindings by propagating more specific native error categories (notably proxy CONNECT failures) and rethrowing them as typed JS error classes, aligning with issue #397’s request for actionable connection-phase failures.

Changes:

  • Added new Rust error variants for proxy CONNECT tunnel failures and proxy auth requirements, and attempted to detect these from underlying hyper_util tunnel errors.
  • Updated Node.js bindings to emit a stable error code prefix and rethrow native errors as specific JS Error subclasses.
  • Pinned tower-http and patched hyper-util to specific git revisions (and updated the lockfile accordingly).

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
impit/src/errors.rs Adds ProxyTunnelError / ProxyAuthRequired and maps TunnelError into these variants.
impit-python/src/errors.rs Handles the new Rust variants in Python error conversion (currently mapped to ProxyError).
impit-node/src/lib.rs Encodes ImpitError variants into string error codes for JS rethrowing.
impit-node/index.wrapper.js Uses rethrowNativeError on failures and exports the new JS error classes.
impit-node/index.js Updates native binding version checks to 0.10.1.
impit-node/errors.js Introduces JS error class hierarchy + rethrowNativeError mapping logic.
Cargo.toml Pins tower-http and adds a git patch for hyper-util.
Cargo.lock Reflects the patched/pinned dependency sources.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Mar 11, 2026
@barjin barjin force-pushed the feat/node-better-errors branch from 980c10f to 2a02cd5 Compare March 11, 2026 13:49
@barjin barjin force-pushed the feat/node-better-errors branch from 2a02cd5 to 0411e3d Compare March 11, 2026 14:11
@barjin
Copy link
Copy Markdown
Member Author

barjin commented Mar 11, 2026

cc @Pijukatel the review request is just for learning purposes, as discussed in the meeting yesterday (but ofc feel free to chip in with tips, I don't really like the implementation 😄 )

@barjin
Copy link
Copy Markdown
Member Author

barjin commented Mar 12, 2026

cc @bliuchak & @daniil-poletaev, please feel free to check if the changes match your expectations

Copy link
Copy Markdown

@daniil-poletaev daniil-poletaev left a comment

Choose a reason for hiding this comment

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

Nice, thank you for taking care of this ❤️

Tested it and it works as expected and returns status code

Copy link
Copy Markdown
Collaborator

@bliuchak bliuchak left a comment

Choose a reason for hiding this comment

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

Thanks!

@barjin barjin merged commit a4b9753 into master Mar 13, 2026
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create dedicated type for HTTP CONNECT errors and pass status code

6 participants