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

Refactor OkHttpUtil #6

Closed
ininmm opened this issue Dec 8, 2020 · 14 comments
Closed

Refactor OkHttpUtil #6

ininmm opened this issue Dec 8, 2020 · 14 comments
Labels
enhancement New feature or request

Comments

@ininmm
Copy link
Collaborator

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 5, 2020, 00:33

OkhttpClient 連結在這

可能需要重新設計一下取得 OkhttpClient 的方式,下面列出需求及目前遇到的問題。

需求

  • 檢查不合法的 URL

  • 每個 link 可能有不同 timeout 跟 cache cookie

  • 以後要有的 check/refresh AccessToken 機制

  • 可能會有的 Link Preview 功能

問題

  1. 目前 OkhttpClient 還是會有反覆創建的問題 (基本上 api instance 保留在 Fragment),這樣做有 leak 的風險,我自己之前的做法是讓 OkhttpClient 保持單例,原因:doc
  2. 獲得 OkhttpClient 的方法有使用到 context ,這邊要解耦一下
  3. 可以確定的是我們應該要有一個方式可以動態變更 OkhttpClient 的設定,ex:根據不同的 request 改 timeout、interceptor 等等,這邊提出可能的作法
@ininmm ininmm added the enhancement New feature or request label Dec 8, 2020
@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 5, 2020, 00:35

changed the description

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 5, 2020, 00:39

@ALL

大家如果有想到什麼問題或是必須要確保的功能在幫我補充上去,感謝!

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 5, 2020, 00:40

marked this issue as related to #1

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @wesely.ong on Nov 5, 2020, 10:34

認同,使用 new OkHttpClient.Builder() 是依照官方建議沒有問題,
個人認為可以避免傳入 context, 而是應該直接傳入 cache (nullable)
若有cache傳入,則加入build config。
若無cache傳入時,則使用 Application 等級來建立可共用的cache

    new Cache(
        new File(MyApplication.application.cacheDir, "normal_cache_name"),
        NORMAL_CACHE_SIZE
    )

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @TakumaLee on Nov 5, 2020, 13:33

基本上同意直接 Application 建立 client,不過如果沒有切換或多個 custom client 的必要的話其實就一個全域 client 也行
refresh token 沒其他可能得跟 context 綁一起的意外的話也可以直接建立在 interceptor 中

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @HsuHsiaoHsuan on Nov 5, 2020, 13:48

@in_in_mm
想請問四個需求大概會是怎麼樣的應用場景呢?
問題的部分有考慮用 Retrofit 去整理網路這一塊嗎?

根據不同 api 改變 timeout 也許可以放在 interceptor。
refresh AccessToken 用 Authenticator?

https://square.github.io/okhttp/3.x/okhttp/okhttp3/Authenticator.html

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @kenhuang1120 on Nov 5, 2020, 15:27

有在考慮使用Retrofit

不過目前主要目標為先完成MVVM雛形

方便大家一起實作

所以這塊應該會先以將OkhttpClient singleton,把傳入 context的部分去掉的方式處理

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @HsuHsiaoHsuan on Nov 5, 2020, 17:12

感覺現在就可以去掉 context:

  1. getTrustlAllClient 沒有用到 Context
  2. getCacheClient 改成傳入 Cache。把 Cache 改成 singleton,在 Application 裡面 init。

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 5, 2020, 17:39

我分成兩段:

  1. Retrofit 是很強大,但是他不支援 websocket (其實有其他 library 幫忙處理掉),必須要回去用 Okhttp 才行,這邊我不太確定中台那邊開的 API 能不能都用 Retrofit 去串,要不要用都可以,不過如果我們想要深入討論的話可以再另外開一個 issue 討論

  2. 我自己是傾向用官方推薦的方式處理掉,timeout 的問題不太確定如果這樣做問題是會變成怎麽處理 interceptor 嗎?

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @HsuHsiaoHsuan on Nov 5, 2020, 23:55

  1. 目前看 swagger 裡面的 API 都是 RESTful 的,中台計畫應該所有都是?如果將來出現 WebSocket,畢竟本來就不一樣應該可以分開管理。會提 Retrofit 是因為可以讓程式碼非常乾淨簡單一目瞭然好維護(反正裡面也都還是 okhttp XD)。

  2. interceptor 可以先攔截到 request 去修改 timeout。如果每一個 API 的 timeout 時間不是太常變動,感覺弄個資料結構先寫好每一個 timeout 時間,看攔截到哪一個 API 進來了就去調整成對應的時間也行 :D

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @PichuChen on Nov 6, 2020, 00:58

Websocket 的部分可以另外使用其他 library, 雖然我現在習慣是用MQTT or Event-Stream

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 6, 2020, 09:22

那這樣看用 Retrofit 應該不會有什麼問題

@ininmm
Copy link
Collaborator Author

ininmm commented Dec 8, 2020

In GitLab by @in_in_mm on Nov 16, 2020, 00:02

mentioned in merge request !17

@ininmm
Copy link
Collaborator Author

ininmm commented Apr 5, 2021

現在先簡單實作一個 Client 在 DI module 裡面

@ininmm ininmm closed this as completed Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant