Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: new hooks #3878

Merged
merged 6 commits into from
Nov 25, 2024
Merged

feat: new hooks #3878

merged 6 commits into from
Nov 25, 2024

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 24, 2024

No description provided.

@ronag ronag force-pushed the new-hooks2 branch 10 times, most recently from 9ab150a to f37d5f4 Compare November 25, 2024 07:54
@ronag ronag force-pushed the new-hooks2 branch 3 times, most recently from 41c3b90 to c22b5c2 Compare November 25, 2024 08:46
@ronag ronag force-pushed the new-hooks2 branch 2 times, most recently from f0cf2a8 to 9d2bf4f Compare November 25, 2024 08:58
@ronag ronag marked this pull request as ready for review November 25, 2024 09:01
@ronag ronag mentioned this pull request Nov 25, 2024
@ronag ronag requested review from mcollina, metcoder95 and ShogunPanda and removed request for mcollina November 25, 2024 09:05
@ronag
Copy link
Member Author

ronag commented Nov 25, 2024

I'm unsure about context in onRequestStart.

A. Is it necessary?
B. Should it live on the controller instead? i.e. the controller is the context. Just add a opaque property the user can use?

@mcollina
Copy link
Member

This looks good to me. Can you update the docs?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

onData: () => {},
onComplete: 'INVALID'
}), new InvalidArgumentError('invalid onComplete method'))
})
Copy link
Member

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The redirect interceptor has some kind of assertion of the handler that does not exist anywhere else and is difficult to have compat with through the wrap/unwrap.

@ronag ronag force-pushed the new-hooks2 branch 2 times, most recently from ec77100 to 78dc2e6 Compare November 25, 2024 10:14
@ronag ronag merged commit 67f3c96 into main Nov 25, 2024
41 of 42 checks passed
@ShogunPanda ShogunPanda deleted the new-hooks2 branch November 25, 2024 12:34
@artur-ma artur-ma mentioned this pull request Nov 26, 2024
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants