✨ v3 (feature): client refactor

[wangjq4214]

Aims

Please provide enough information so that others can review your pull request:

Move client.go to client/ to make project structure more tidy. Re-create client by inspiring by go-resty/resty or axios/axios.

Details

Explain the details for making this change. What existing problem does the pull request solve?

This PR is related to #1906.

TODO List

• [X] Core request process, hook and plugin mechanism
• [X] URL and baseurl merge mechanism
• [X] Header setting
• [X] Query parameter setting
• [X] User agent setting
• [X] Cookie setting
• [x] Path parameter setting
• [x] Form data support
• [x] JSON body, XML body, Raw body, Files body
• [x] Refer setting
• [x] Unmarshal
• [x] Chain API (processing)
• [x] API like axios
• [x] Timeout
• [x] Response save to file or io.Writer
• [x] TLS support
• [x] Proxy
• [x] Retry (https://github.com/gofiber/fiber/pull/1972) (Thanks for it, I'll continue this work)
• [x] Throttle
• [x] Extending the axios-like API options
• [x] Redirect
• [x] Improve test coverage (95% up, 96.7% now)
• [x] Add benchmarks
• [x] Cookie jar
• [x] Logger (A simple impl, may be improve)
• [x] ~~Trace~~ (Need PR for fasthttp)
• [ ] Add API docs and examples

[wangjq4214]

Can you add some features that I haven't thought of? Please! @efectn

[efectn]

Can you add some features that I haven't thought of? Please! @efectn

Sure ill take a look

[wangjq4214]

The refactoring work is done basically, then I will finish the rest of the work in TODO list and add unit tests.

Please do a part of the code review work first. Thanks! @efectn @ReneWerner87

[efectn]

I reviewed API and it's fine to me. And we may extend request setter options more.

Also what's https://github.com/wangjq4214/fiber/blob/v3-client/client/plugins.go?

[wangjq4214]

We should add README.md to explain new client. We can also give some examples.

Should I create a new README.md in the client package?

[wangjq4214]

I reviewed API and it's fine to me. And we may extend request setter options more.

Also what's https://github.com/wangjq4214/fiber/blob/v3-client/client/plugins.go?

More options

I will further expand the options after I finish the remaining three features.

Plugins

At first I wanted to provide users with the ability to change the request process through the plugin.

For example like as follows:

package client

import (
	"context"
	"time"
)

type TimeoutPlugin struct {
	time int
}

func (t *TimeoutPlugin) Name() string {
	return "timeout-plugin"
}

func (t *TimeoutPlugin) Check() bool {
	return t.time != 0
}

func (t *TimeoutPlugin) GenerateExecute(f ExecuteFunc) (ExecuteFunc, error) {
	return func(ctx context.Context, c *Client, r *Request) (*Response, error) {
		ctx, cancel := context.WithTimeout(ctx, time.Duration(t.time)*time.Second)
		defer func() { cancel() }()
		return f(ctx, c, r)
	}, nil
}

func NewTimeoutPlugin(time int) *TimeoutPlugin {
	return &TimeoutPlugin{time: time}
}

var _ Plugin = (*TimeoutPlugin)(nil)

But now I think it's an overdesign. Any Suggestions？

[efectn]

I reviewed API and it's fine to me. And we may extend request setter options more. Also what's https://github.com/wangjq4214/fiber/blob/v3-client/client/plugins.go?

More options

I will further expand the options after I finish the remaining three features.

Plugins

At first I wanted to provide users with the ability to change the request process through the plugin.

For example like as follows:

package client

import (
	"context"
	"time"
)

type TimeoutPlugin struct {
	time int
}

func (t *TimeoutPlugin) Name() string {
	return "timeout-plugin"
}

func (t *TimeoutPlugin) Check() bool {
	return t.time != 0
}

func (t *TimeoutPlugin) GenerateExecute(f ExecuteFunc) (ExecuteFunc, error) {
	return func(ctx context.Context, c *Client, r *Request) (*Response, error) {
		ctx, cancel := context.WithTimeout(ctx, time.Duration(t.time)*time.Second)
		defer func() { cancel() }()
		return f(ctx, c, r)
	}, nil
}

func NewTimeoutPlugin(time int) *TimeoutPlugin {
	return &TimeoutPlugin{time: time}
}

var _ Plugin = (*TimeoutPlugin)(nil)

But now I think it's an overdesign. Any Suggestions？

I think we don't need a plugin mechanism. Hooks are enough for extendability.

Should I create a new README.md in the client package?

Yes

[efectn]

What do you think about tracing? I don't think Fasthttp client supports tracing

[wangjq4214]

What do you think about tracing? I don't think Fasthttp client supports tracing

You are right, I will remove this feature. Maybe a PR needs to be submitted for fasthttp client.

[wangjq4214]

The unit test part of the client package references fiber, and the unit test part of fiber needs to reference client, forming a circular reference. How do you think it is better to modify it? @ReneWerner87 @efectn

[efectn]

The unit test part of the client package references fiber, and the unit test part of fiber needs to reference client, forming a circular reference. How do you think it is better to modify it? @ReneWerner87 @efectn

What tests need fiber client? we can replace them with fasthttp client or http client

[wangjq4214]

The unit test part of the client package references fiber, and the unit test part of fiber needs to reference client, forming a circular reference. How do you think it is better to modify it? @ReneWerner87 @efectn

What tests need fiber client? we can replace them with fasthttp client or http client

Fine. I'll fix it.

[efectn]

Can you add https://github.com/wangjq4214/fiber/blob/v3-client/redirect_test.go#L235 back

[wangjq4214]

Can you add https://github.com/wangjq4214/fiber/blob/v3-client/redirect_test.go#L235 back

Done

[efectn]

Hey when do you plan to complete client? Perhaps i can help you to add more tests and benchmarks

[wangjq4214]

Hey when do you plan to complete client? Perhaps i can help you to add more tests and benchmarks

I've been busy with my paper these last few weeks, and I'll finish it next week.

[efectn]

Looks like we need little additions to increase test coverage (especially for tls and proxy), i can do them after my exam week. Also Fasthttp doesn't support cookie jar, we can add that as TODO. What do you think about redirect and logger @wangjq4214

[wangjq4214]

Looks like we need little additions to increase test coverage (especially for tls and proxy), i can do them after my exam week. Also Fasthttp doesn't support cookie jar, we can add that as TODO. What do you think about redirect and logger @wangjq4214

Maybe we can implement a simple cookie jar by ourselves. In addition, what information do you think the logger should contain.

[efectn]

Looks like we need little additions to increase test coverage (especially for tls and proxy), i can do them after my exam week. Also Fasthttp doesn't support cookie jar, we can add that as TODO. What do you think about redirect and logger @wangjq4214

Maybe we can implement a simple cookie jar by ourselves. In addition, what information do you think the logger should contain.

I'm not really sure about we need logger. Cant we implement it by using hooks?

[wangjq4214]

I'm not really sure about we need logger. Cant we implement it by using hooks?

My original idea was to use hooks to implement logger, so as to output some necessary information

[efectn]

People may want to use other logging libs like zerolog, zap etc. I think we should provide simple doc instead of adding an internal logger.

[efectn]

@wangjq4214 i see some commented tests. Are they failing at the moment?

[wangjq4214]

@wangjq4214 i see some commented tests. Are they failing at the moment?

Thanks for your commit. Sorry, I forget about it and I'll take a look and finish cookie jar and logger tonight. I'm so busy these weeks.

[efectn]

@wangjq4214 i see some commented tests. Are they failing at the moment?

Thanks for your commit. Sorry, I forget about it and I'll take a look and finish cookie jar and logger tonight. I'm so busy these weeks.

No problem. If you don't have much time, i can help you about some TODOs

[efectn]

I'll complete the remaining tasks and perhaps will merge PR soon. We can prepare docs later.

[efectn]

hi @wangjq4214. Is cookiejar done and ready to writing tests? I wanted to ask before trying to write tests for it.

[wangjq4214]

hi @wangjq4214. Is cookiejar done and ready to writing tests? I wanted to ask before trying to write tests for it.

Oh, I feel very sorry for it, I almost forget this PR.

Let me check it.

[efectn]

Everything besides documentation is done. Ready for review.

[ReneWerner87]

will do the code review tomorrow, sorry for the latency

[efectn]

Todo list:

• [ ] Prepare docs.
• [x] Find performant solution for fasthttp.HostClient. If we use common HostClient for requests, it will cause problems since listener is shared as well. If we create hostclient per request, performance will be worse.