Update client to use injected Axios Instance #22

gnujoow · 2023-01-05T11:39:11Z

2안과 1안에 대해서 각각 코멘트 남겨두었습니다. 아래로 내려가면서 읽어보시면 되겠습니다.

보시고 자유롭게 의견주세요

gnujoow

2안) class형태로 작성하는 케이스입니다.

이 경우

export const client = new CosmosQueryClient({
    baseURL: lcdList["osmosis-1"],
  });

위처럼 선언하고 다른 파일에서 import해서 사용할 수 있습니다.

examples/application/pages/example1.tsx

packages/core/src/cosmosQueryClient.ts

gnujoow

1안은 이전처럼 사용하면됩니다. 다만 baseURL을 받는것이 아니라 axiosConfig를 인자로 받습니다.
baseURL 만 넘기는 경우에는 번거롭진 않겟지만 복잡한 header를 계속 선언하는 경우에는 조금더 번거로울 수 있을거같아요

examples/application/pages/example1.tsx

packages/core/src/cosmosQueryClient.ts

junhoyeo · 2023-01-05T11:54:14Z

저는 axios instance 자체를 optional 인 인자로 받고, 이 값이 없을 때만 미리 선언된 defaultAxiosInstance 등을 불러와 사용하는 형태가 좋을 것 같아요.

gnujoow · 2023-01-05T12:47:40Z

이 값이 없을 때만 미리 선언된 defaultAxiosInstance 등을 불러와

좋은 의견감사합니다.defaultAxiosInstance아이디어는 진짜 좋은것 같아요. 다만, 이 아이디어를 반영하려면 defaultAxiosInstance의 baseURL이 있어야할것 같은데, 하나 정해보면 어떨까요? atom? osmosis?

junhoyeo · 2023-01-06T05:18:48Z

@gnujoow 와 구두로 논의한 내용 정리합니다.

Axios Client

Axios 인스턴스(클라이언트)를 외부에서 주입하는 방식으로 구현합니다. 이 경우, 라이브러리를 사용하는 앱에서 같은 인스턴스를 여러 번 선언할 필요 없이, 재사용할 수 있습니다(직접 쿼리를 하는 경우 등).
axios 패키지는 현재 시점의 최신 버전으로 고정합시다. 이 버전을 필요에 따라 변경하는 것은 라이브러리를 사용하는 앱의 의무로, resolutions 등으로 고정하는 등의 방식을 취할 수 있습니다.

사용 예(1안 vs 2안) 👉 1안으로 합의

@gnujoow 어느 쪽이든 좋으나, 기존 인터페이스를 가져갈 수 있어 1안이 어떨까 싶습니다.
@junhoyeo 필요한 경우, 앱이 필요한 메소드를 import 등으로 동적으로 로딩할 수 있기 때문에 저도 1안을 지지합니다.

dongwu-kim · 2023-01-06T06:14:41Z

구두로 전달받았습니다!

AxiosInstance 를 받는 형태로 간다면 큰 이견 없습니다.
사내 프로젝트에서 어떤 문제가 있을지 좀 생각해봤는데 위에서 제시한 버전 변경에 의해 발생할만한 문제가 크지는 않을것 같아요.

정말 문제가 있더라도 최대 3시간, 보통 그보다 적은 시간 내에 다 해결할 수 있을것 같습니다.
기존 Ion-interface의 경우는 이미 1.1.2 정도로 알고 있어서 큰 수정사항은 없을것 같아요.

dashboard의 client 에서는 Keplr-wallet에서 기존 axios 를 constructor에 추가하는 코드를 전부 날려두어 다른 의존성에 의한 문제가 발생할 것 같지는 않습니다.

client 형태로 만드는 것도 매력적이지만, 기존 사용법과 큰 괴리가 없는 형태로 작업하는게 지금은 더 중요할 것 같습니다.

RPC 쿼리를 지원해야 할 때 아마 Cosmjs QueryClient(Custom query 등록)를 활용하는 편이 아마 좀 더 빠르게 구현하는 방법이 될 것 같은데 그 때 client 형태로 한 번 만들어봐도 좋을것 같습니다! 뭔가 Cosmjs base 로 작업하다 나중에 좀 더 여유가 생기면 제거하는 방식으로? 작업해보면 좋을것 같아요.

gnujoow · 2023-01-06T06:16:06Z

@junhoyeo @dongwu-kim
두분 의견 감사합니다. AxiosInstance를 전달 받고, 위의 1안의 형태로 작업하는데 합의가 된것 같습니다. 이 방향으로 작업을 진행하도록 하겠습니다.

gnujoow

update된 내용입니다. 중복되는 내용들이 많아서 아래 코멘트를 확인해주세요

todo

test
default instance가 필요한 경우를 결정하고 반영하기

discussion

2-3개의 쿼리만 동작확인을 했습니다. 전부다 동작하는지 확인하려면 시간이 꽤 걸릴거 같네요.. 테스트코드가 있으면 어떨까 합니다.
default instance가 필요한 경우가 있을까요?

gnujoow · 2023-01-09T03:23:20Z

examples/application/pages/index.tsx

+  const client = axios.create({
+    baseURL: lcdList["osmosis-1"],
+    paramsSerializer: {
+      serialize: (params: Record<string, any>) => {
+        return stringify(params, { allowDots: true });
+      },
+    },
+  });
+  const { getAccounts } = getCosmosQuery(client);


사용법이 앞으로 위와 같은 방식으로 변경됩니다.

사용되는 서비스에서 AxiosInstance를 만들고 parameter로 전달해주세요.
Axios는 v1.x를 사용해야합니다.

paramsSerializer를 덮어쓰는 방법이 있을지

gnujoow · 2023-01-09T03:25:02Z

packages/core/src/apis/cosmwasm/wasm/index.ts

+    const msg = JSON.stringify(contractQueryInterface);
+    const query = Buffer.from(msg).toString("base64");
+
+    if (isWasmd) {
+      return (
+        await instance.get(
+          `/cosmwasm/wasm/v1/contract/${contractAddress}/smart/${query}`
+        )
+      ).data;
+    }
+
    return (
-      await instance(baseURL).get(
-        isWasmd
-          ? getWasmDUrlFromObj(contractAddress, contractQueryInterface)
-          : getUrlFromObj(contractAddress, contractQueryInterface)
-      )
+      await instance.get(`/wasm/v1/contract/${contractAddress}/smart/${query}`)


fyi. @dongwu-kim 정확한 이유는 밝히지 못했는데 getWasmDUrlFromObj, getUrlFromObj 를 사용헀을 경우 window.formdata를 못찾는다는 런타임에러가 계속 발생하더라구요 해서 위처럼 변경했습니다.

왜 에러가 났는지 모르겠네요 -__ -

gnujoow · 2023-01-09T03:27:05Z

packages/core/src/getOsmosisHistoricalQuery.ts

+  const setInstance = <T>(callback: (queryInstance: AxiosInstance) => T) => {
+    return callback(instance);
+  };


기존 getOsmosisHistoricalQuery에서는 default instance를 사용하도록 강제되었었는데 다른 query와 마찬가지로 instance를 받아서 사용하도록 변경합니다.

gnujoow · 2023-01-09T03:27:35Z

packages/core/src/getOsmosisHistoricalQuery.ts

+  /**
+   * todo
+   * make axios call with default instance when instance is not given
+   */


default instance를 넘겨주려고 헀는데 런타임 에러가 계속 발생해서 이유를 파악중입니다.

add suggestion for client update

ed72382

gnujoow commented Jan 5, 2023

View reviewed changes

examples/application/pages/example1.tsx Outdated Show resolved Hide resolved

examples/application/pages/example1.tsx Outdated Show resolved Hide resolved

packages/core/src/cosmosQueryClient.ts Outdated Show resolved Hide resolved

gnujoow marked this pull request as ready for review January 5, 2023 11:46

junhoyeo self-requested a review January 6, 2023 05:14

junhoyeo changed the title ~~add suggestion for client update~~ Update client to use injected Axios Instance Jan 6, 2023

gnujoow closed this Jan 6, 2023

gnujoow reopened this Jan 6, 2023

gnujoow marked this pull request as draft January 6, 2023 06:16

revert suggest example

b7a2faf

junhoyeo requested a review from Ryz0nd January 6, 2023 06:21

gnujoow force-pushed the feature/upgrade branch from fa65775 to 0667737 Compare January 6, 2023 06:42

gnujoow added 2 commits January 6, 2023 15:43

update getQuery function to receive Axios Instance parameter

e4138ef

update OsmosisHistoricalQuery to receive custom Axios Instance

7397596

gnujoow force-pushed the feature/upgrade branch from 0667737 to 7397596 Compare January 6, 2023 07:16

gnujoow added 3 commits January 6, 2023 18:17

bump axios to v1.x

db94f55

refactoring getQuerySmartContract to avoid runtime error

d1e46d2

update osmosisHistoryQuery

ca813c7

gnujoow marked this pull request as ready for review January 9, 2023 03:21

gnujoow commented Jan 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update client to use injected Axios Instance #22

Update client to use injected Axios Instance #22

gnujoow commented Jan 5, 2023 •

edited

Loading

gnujoow left a comment

gnujoow left a comment

junhoyeo commented Jan 5, 2023

gnujoow commented Jan 5, 2023 •

edited

Loading

junhoyeo commented Jan 6, 2023 •

edited by gnujoow

Loading

dongwu-kim commented Jan 6, 2023 •

edited

Loading

gnujoow commented Jan 6, 2023

gnujoow left a comment •

edited

Loading

gnujoow Jan 9, 2023

gnujoow Jan 9, 2023

gnujoow Jan 9, 2023

gnujoow Jan 9, 2023

gnujoow Jan 9, 2023

Update client to use injected Axios Instance #22

Are you sure you want to change the base?

Update client to use injected Axios Instance #22

Conversation

gnujoow commented Jan 5, 2023 • edited Loading

gnujoow left a comment

Choose a reason for hiding this comment

gnujoow left a comment

Choose a reason for hiding this comment

junhoyeo commented Jan 5, 2023

gnujoow commented Jan 5, 2023 • edited Loading

junhoyeo commented Jan 6, 2023 • edited by gnujoow Loading

Axios Client

사용 예(1안 vs 2안) 👉 1안으로 합의

dongwu-kim commented Jan 6, 2023 • edited Loading

gnujoow commented Jan 6, 2023

gnujoow left a comment • edited Loading

Choose a reason for hiding this comment

todo

discussion

gnujoow Jan 9, 2023

Choose a reason for hiding this comment

gnujoow Jan 9, 2023

Choose a reason for hiding this comment

gnujoow Jan 9, 2023

Choose a reason for hiding this comment

gnujoow Jan 9, 2023

Choose a reason for hiding this comment

gnujoow Jan 9, 2023

Choose a reason for hiding this comment

gnujoow commented Jan 5, 2023 •

edited

Loading

gnujoow commented Jan 5, 2023 •

edited

Loading

junhoyeo commented Jan 6, 2023 •

edited by gnujoow

Loading

dongwu-kim commented Jan 6, 2023 •

edited

Loading

gnujoow left a comment •

edited

Loading