Skip to content
This repository was archived by the owner on Dec 24, 2025. It is now read-only.

fix: make request_builder_hook configurable - #13

Draft
mocchira wants to merge 1 commit into
masterfrom
ysh/make-request_builder_hook-configurable
Draft

fix: make request_builder_hook configurable#13
mocchira wants to merge 1 commit into
masterfrom
ysh/make-request_builder_hook-configurable

Conversation

@mocchira

Copy link
Copy Markdown
Contributor

This PR makes request_builder_hook configurable for https://github.com/tailor-inc/platform-core-services/issues/8902

@mocchiramocchira self-assigned this Aug 18, 2025
@mocchira
mocchira requested a review from CopilotAugust 18, 2025 05:09

CopilotAI left a comment

Copy link
Copy Markdown

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 makes the request_builder_hook configurable by enabling it to be passed through from WebOptions to the deno_fetch extension, addressing the TODO comment that previously indicated it wasn't being used.

  • Changes the error type from deno_core::error::AnyError to deno_error::JsErrorBox in hook signatures
  • Removes the TODO comment and enables the request_builder_hook to be passed from WebOptions to deno_fetch
  • Updates import statements to use the new error type

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

FileDescription
src/runtime_builder.rsUpdates the hook parameter type in with_web_request_builder_hook method
src/ext/web/options.rsChanges the error type in WebOptions struct and updates imports
src/ext/web/mod.rsEnables the request_builder_hook to be passed through to deno_fetch instead of being hardcoded to None

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@remiposoremiposo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mocchira@remiposo