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

Feature/【修正】vueのメソッドに非同期処理追加 #28

Merged
merged 14 commits into from Jul 25, 2019

Conversation

@atlansien
Copy link
Owner

atlansien commented Jul 24, 2019

行なったこと

Input.vue

postTodoButtonasync/awaitを追加しました

TodoDialog.vue

putTodoButtonasync/awaitを追加しました

なぜ非同期処理を追加したか

非同期処理をつけずに実行した場合、サーバー側でエラーが発生してフロント側は問題なく実行された場合、サーバー側とフロント側のデータに差異が出てしまい、整合性が取れなくなる可能性があるため

postTodoButton() {
this.postTodo({ newTitle: this.title, newBody: this.body });
async postTodoButton() {
await this.postTodo({ newTitle: this.title, newBody: this.body });

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 24, 2019

レスポンスのステータスコードを確認してエラーが帰ってきたときは、this.title, this.bodyの内容はそのままで、なぜエラーが発生したかユーザーに知らせた方が良いので、

ステータスコードで条件分岐を行って適切な処理を行うようにしたほうがよいと思います

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

ステータスコードはaxiosのレスポンスデータから抜き出してデータに渡すような感じでいいでしょうか?

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

エラーのハンドリングができれば良いので、ステータスコードを使うでも良いですし、レスポンス内に error を表すプロパティが含まれていればそれを使うでも問題ないです。

ここでやるべきこととしては、ユーザーに正しく作成・更新できなかったことを知らせることなので。

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

了解です、今回はレスポンスから返ってくるエラーコードをそのまま使用しました(内容にステータスコードも含んでいたので)
エラーの場合Input.vueだとこのようになります。
スクリーンショット 2019-07-25 10 59 36

それに伴い、actionsのメソッドとテストを修正しました、テストはクリアしました。

また、今回もgit resetの関係でforce pushを使用しました。
(参考を見ると、当ブランチをマージして、再プルリクを送るのが綺麗なやり方らしいですが、レビュー中のためforce pushを使用しました`)
参考: https://teratail.com/questions/92539

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

また、今回もgit resetの関係でforce pushを使用しました。

今回 git reset 使った理由は何でしょうか?
force pushはあまりクセにならないように気をつけていただけたらと思います^^;

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

git pushした後にコードを修正しようと思い、手直しするより特定のログまで戻した方が早いなと思いresetしました。rejectされた時にローカルでresetしたらリモートとコンフリクトすることを思い出したので、気をつけたいと思います

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

バージョンを戻したい場合は、checkout を使うことで特定のファイルを特定のバージョンに戻すことが出来るのでこちらなどを活用すると良いかと思います。
(resetだとコミットの変更内容が全て変更される + コミット履歴が変わるので危険)

https://qiita.com/rch1223/items/9377446c3d010d91399b#%E7%89%B9%E5%AE%9A%E3%81%AE%E3%83%95%E3%82%A1%E3%82%A4%E3%83%AB%E3%81%AE%E3%81%BF%E3%82%B3%E3%83%9F%E3%83%83%E3%83%88%E3%81%AE%E3%83%90%E3%83%BC%E3%82%B8%E3%83%A7%E3%83%B3%E3%82%92%E6%88%BB%E3%81%97%E3%81%9F%E3%81%84


rejectされた時にローカルでresetしたらリモートとコンフリクトすることを思い出したので、気をつけたいと思います

今回もこのまま最終的にマージしてもOKですが、
次回以降はforce pushしたもに対してはその時点でそのプルリクでの添削は強制終了として
別プルリクにて新しくつくってもらおうと考えています。
(再発防止策)

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

了解しました

const editData = {
id: this.todo.id,
title: this.title,
body: this.body
};
this.putTodo(editData);
await this.putTodo(editData);
@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Jul 24, 2019

2点コメントしたのでご確認お願いしますmm

@atlansien atlansien force-pushed the feature/add-promise-to-the-code branch from b9c6cef to 4ffc8a0 Jul 25, 2019
this.postTodo({ newTitle: this.title, newBody: this.body });
async postTodoButton() {
try{
await this.postTodo({ newTitle: this.title, newBody: this.body });

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

インデントが崩れています
(tryとawaitが同じインデント位置になっていて、事前自前レビューをするか、コミット時にlintをかけるようにすることで、ここらへんの小さいミスは防げるかと思います)

this.title = "";
this.body = "";
}catch (error){
this.isError = true;
this.errorMsg = error.message

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

他ではセミコロンを使っているので、統一してセミコロンをつけるようにお願いします。
(こちらの小さいミスも、コミット時にlintをかけるようにすると解決する問題かなと思います。)

参考記事: https://qrunch.net/@tomoreb/entries/MeIEqIj00N6fDdnU

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

修正しました、当記事参考にします

@@ -9,7 +9,7 @@ export default {
const todoData = res.data;
commit("setTodos", todoData);
} catch (error) {
throw new Error("APIエラーが発生しました");
throw new Error(error);

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

【質問】

ここの error はstring型でしょうか?それともかErrorオブジェクトでしょうか?
もしErrorオブジェクトであれば、new Error で二重にエラーオブジェクトに囲まずに、そのまま throw error で良いかなと思いました。

もしstring型であれば、error ではなく、 errorMessage のようにstring型であることが推測できる引数名を使うことをオススメします。

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

オブジェクト型です。
throw errorをした場合、エラーテストが上手くいかなかったので、出力するときはvue側で調整すればいいかなと思いthrow new Error(error)を採用しました。

  • throw errorを使用した場合

スクリーンショット 2019-07-25 11 27 07

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

throw errorをした場合、エラーテストが上手くいかなかったので、出力するときはvue側で調整すればいいかなと思いthrow new Error(error)を採用しました。

エラーテストがうまく行かなかった理由はなぜかした上での修正であれば問題ないですが如何でしょうか?

  1. エラーになった理由
  2. 今回の修正によってどうしてテストが通るようになったか

の2点をご回答いただけると幸いですmm

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

おそらくthrow errorの場合はErrorオブジェクトとして返してはいないので、モックで検知する事ができなかったのかな?と思います。
モック内で

if (mockError) {
  const error = "error"
  throw error;
}

といった方法を試しましたが、これはエラーテストの意味がないと思いやめ、今のテストコードより良い方法が思いつかなかったので今回のやり方にしました

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

おそらくthrow errorの場合はErrorオブジェクト

console.logで確認したらエラーオブジェクトでした。
(見ている場所がもし違ってたらすみません)

モックで検知する事ができなかったのかな?

手元で試してうまく動いたので、以下のコードをプルリクのFile Changedや参考記事の内容を元に修正お願いします。

duyoji/TODO_APP_WITH_EXPRESS_AND_MYSQL#1

@@ -22,7 +22,7 @@ export default {
const todoData = res.data;
commit("addTodo", todoData);
} catch (error) {
throw new Error("APIエラーが発生しました");
throw new Error(error);
@@ -34,7 +34,7 @@ export default {
const todoData = res.data;
commit("updateTodo", todoData);
} catch (error) {
throw new Error("APIエラーが発生しました");
throw new Error(error);
@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Jul 25, 2019

#28 (comment)

上記質問に対する回答が漏れているので回答お願いしますmm

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Jul 25, 2019

質問2点以下にまとめたのでご確認お願いいたしますmm

#28 (comment)

#28 (comment)

@@ -69,7 +69,7 @@ describe("TEST acitons.js", () => {
mockError = true;

await expect(actions.postTodo({ commit: jest.fn() }, {})).rejects.toThrow(
"APIエラーが発生しました"
"Erorr"

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Jul 25, 2019

タイポによってテストが通って居ない状態になっているので、
修正後はテストを実行して通ることを確認してから再レビュー出すようにお願いしますmm

image


タイポ修正後テストが通ったのを確認しました。

image


繰り返しになりますが、修正を終えて再レビュー依頼出す前は

  • 事前自前レビュー
  • テストがちゃんと通るかまで確認

するようにお願いいたしますmm

This comment has been minimized.

Copy link
@atlansien

atlansien Jul 25, 2019

Author Owner

すいません、typoでテストに通らないかチェックしていた分まで間違えてadd、コミットしていました。
修正しましたのでよろしくお願いします。

@atlansien atlansien merged commit 2b47933 into develop Jul 25, 2019
@atlansien atlansien deleted the feature/add-promise-to-the-code branch Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.