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

Todoを新規作成する機能 #10

Merged
merged 21 commits into from Apr 26, 2019

Conversation

@keisuke3
Copy link
Owner

keisuke3 commented Apr 17, 2019

Todoを新規作成する機能を実装したので、レビューをお願いします。
・タイトル、コメントのどちらか、もしくは両方が未入力の場合、エラーメッセージが出るようにしました。
・タスク入力部分のデータは、storeで管理し、storeとcomponentsをマッピングしています。

スクリーンショット 2019-04-17 18 18 16

今回、モジュールを使用したのですが、データの管理が難しいと感じました。
Qiitaなどで、調べてみたのですが、いまいち正解がわからなかったので、質問させてください。
基本的には、components固有のデータはモジュールで管理して、componentsをまたぐデータは、storeのstateで管理するという感じで良いのでしょうか?

const store = new Vuex.Store({
state: {
todos: []
todos: [],
newTitle: '',

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

基本的には、components固有のデータはモジュールで管理して、componentsをまたぐデータは、storeのstateで管理するという感じで良いのでしょうか?

そのような認識で良いと思います^^

その点を考慮すると、newTitleやnewBodyなど入力フォームで使うデータは、入力フォームのコンポーネントに閉じ込めた方が良いと思うので、storeでnewTitleやnewBodyの管理はしないほうが良いかと思います。
(1つのコンポーネントでしか使わないものをどんどんstoreに詰め込むと、storeがどんどん肥大化して管理が大変になってくるので。)

state: {},
mutations: {},
actions: {
postTodo({ state, commit, rootState }) {

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

store内にセットしているデータではなく、引き数経由で外からTodoデータを受け取るようにし他方が良いかと思います。

理由はいかに書いたのもそうですが、

https://github.com/keisuke3/creating_api_express_mysql/pull/10/files#r276171736

引き数経由でTodoデータを受け取れるようにすることで、依存先が少なくなるというのも理由です。

@@ -30,6 +74,9 @@ const store = new Vuex.Store({
})
}
},
modules: {

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

今回、モジュールを使用したのですが、データの管理が難しいと感じました。

Vue経験が乏しいので僕自身moduleを正しく理解していないですが、
モジュールとして別ファイルで監理できると思うので、

このファイル内にTextareaモジュールを直接埋め込むより、別ファイルにstores/modules/Textarea.js (仮名) のように切り分けたほうが良いかなと思います。

そうすることで、以下のようなメリットがあります。

  • store.js内のコードが短くなり読みやすくなる
  • 複数ファイルで共有できるようになる
  • モジュールごとのテストが書きやすくなる

など


ただ、Textareaのコンポーネント内にtitleやbodyを管理するようにしたほうが個人的には良いかなと思います。
(理由は以下の通り)

https://github.com/keisuke3/creating_api_express_mysql/pull/10/files#r276171736

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Apr 17, 2019

store.js(Vuex部分)はデータ管理などバックエンドのWebフレームワークで言うとModelのような立ち位置になると思うので、テストコードを書いたほうが良いかと思います。

参考記事

https://vuex.vuejs.org/ja/guide/testing.html
https://qiita.com/suzu-4/items/cf4f52049c681f4889e5

テストを書くことで、テストが書きやすい形式でstoreを記述することを心がける必要が出てくるので、
storeの設計がしっかりしやすくなります。

また、storeの動作確認もブラウザ越しに手動でデータの入力、actionの実行をしなくても、テストを十石することで、意図通り確認できるので、テストを書くと開発効率もあがります。


コンポーネントなどView周り(表示周り)と比較すると、ロジック部分は正しく動かないとサービス自体が止まってしまうので、表示周りのテストコードを書くよりは優先度が高くなります。
(表示が崩れても、ロジック部分が正しく動けばサービスは使えるため)

export default {
name: 'Textarea',
computed: {
...mapState([

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

https://github.com/keisuke3/creating_api_express_mysql/pull/10/files#r276171736

上記でも書きましたが、入力したtitle, bodyの値はコンポネント内で閉じ込めた方が影響範囲を、このコンポネント内だけに閉じ込められるので、閉じ込めた方が良いかなと思います。


そして、postTodoを実行する際は、このコンポネントに閉じ込めたbody, titleをpostTodoの引き数に渡してあげることで、stateでわざわざnewTitle, newBodyを管理しなくてもよくなるようになります。

https://github.com/keisuke3/creating_api_express_mysql/pull/10/files#diff-5914ca51ee5ff9f1b56c437e12bbbbf5R6

import axios from 'axios'

Vue.use(Vuex)

const API_URL = 'http://localhost:3000/api/todos'

const Textarea = {

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

モジュールとして切り分けるなら、Textareaではなく、Todoというモジュール名にして、

todosの管理や、fetchTodos, postTodo, putTodo, deleteTodoなどのTodoに関する操作周りをTodoモジュールで管理すると良いと思いました。


Textareaだとユーザーの入力に関することのみを行うような印象を名前から持ちますが、
実際にはTodo周りの操作を行うかと思うので、モジュール名と行っていることが微妙に乖離しているような気がしました。

逆にTodoという名前でモジュールを作ることで、Todoに関連するモジュール(データ管理や操作)が、名前から判断できます。

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

Todoモジュールを別ファイルで作成した場合は const API_URL = 'http://localhost:3000/api/todos' の部分もTodoモジュールに移せるようになるかと思います。
(api/todos という名前より、todoに関連するAPIなので、これもTodoモジュールファイルに閉じ込めるのが適切)

state.errorMsg = ''
state.todos.push(todoData)
},
inputRequired(state) {

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 17, 2019

https://github.com/keisuke3/creating_api_express_mysql/pull/10/files#r276181263

コンポネント内でtitle, body値を閉じ込めることで、 inputRequired , changeNewTitle changeNewBody と、 state.newTitle state.newBody がstoreから削除できて、

Textareaコンポーネントに閉じ込められるようになります。
(Textareコンポーネントがより独立したコンポーネントとなる。)

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Apr 17, 2019

いくつかコメントしたのでご確認お願いしますmm


ガーッと書いたので、少しまとまりのないコメントになっているかもです^^;
特に伝えたかったのは、

  • コンポーネントをまたがない、データに関してはstoreではなくコンポーネントに閉じ込めた方が良い
  • 複数のコンポーネントで共有するデータに関してはstoreで一元管理したほうが良い(データ内容を同期する)
  • Vuex(store, actions)部分は意図通りに動くことを確認するためにテストコードを書いたほうが良い(テストが書きやすい設計を意識するようにもなる。)
@keisuke3

This comment has been minimized.

Copy link
Owner Author

keisuke3 commented Apr 20, 2019

公式ドキュメントを参考にしてJestを導入してテストを実行してみたのですが、以下のエラーが出てしまいます。
スクリーンショット 2019-04-20 21 23 15

エラー文を参考に調べてみたのですが、解決策を見つける事ができませんでした。
.babelrcの記述が間違っているか、nmpでインストールするものが足りていないのかと思っているのですが、何かアドバイスをいただけないでしょうか?

@keisuke3

This comment has been minimized.

Copy link
Owner Author

keisuke3 commented Apr 25, 2019

修正をしたので、レビューをお願いします。
変更した点についてですが、
・モジュールの使用はやめて、コンポーネントをまたがないデータはコンポーネントで管理するようにしました。
・storeを要素毎にファイル別に管理するようにしました。
変更のあるファイルは、
・src/components/parts/NewTodo.vue
・src/store
・test/unit/specs ・test/unit/store
です。
テストに関しては、NewTodo.vueの単体テスト、storeのactionsのテストを行いました。
他のmutationsなどのテストは、変更差分が大きくなるかと思ったので、今回は行わずレビュー依頼させていただきました。

<textarea placeholder="コメント" class="new-body" v-model="newBody" cols="30" rows="2"></textarea>
<button v-on:click="postTodo" class="add-button">追加</button>
<p v-if="errorFlag" class="error-msg">{{ errorMsg }}</p>
<input placeholder="タイトル" class="new-title" v-model="newTitle" new-title>

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

【質問】

末尾の v-model="newTitle" new-title>new-title の部分はおそらくvue特有の気泡になるかと思うのですが、 new-title で行っている内容はどのようなことでしょうか?
(v-model="newTitle" はコンポーネント内で保持している newTitle かなと推測出来るのですが、new-title はどこから来ているのか気になり質問しました。)

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 25, 2019

Author Owner

new-titleの部分は、テストコードを書く時に、inputに仮の値を設定するために使用していました。wrapper.find('[new-title]').setValue('testTitle')
'[new-title]'の部分を最初は、'input'にしていたのですが、inputは見つかりませんとエラーが出ました。調べたところ上記の方法を見つけたので、使用していました。テストコードの為に書いているだけで、おそらく意味はないと思います。
今、wrapper.find('input').setValue('testTitle')で実装するとテストが通ったので、おそらく最初は自分が文法ミスをしていたのだと思います。最初のやり方だと分かりづらいので、
wrapper.find('input').setValue('testTitle')の方法で実装するようにしました。

data() {
return {
newTitle: '',
newBody: '',
errorFlag: false
errorFlag: false,
errorMsg: '入力は必須です'

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

errorMsg に関しては、動的に値が変わることがなさそうなので、templateに「入力は必須です」と直接埋め込んでも良いのかなと思いました。


もしくは、newTitle が未入力のときは newTitleは必須ですnewBody が未入力のときは newBodyは必須です、 両方未入力のときは title, bodyは必須です のように動的にエラーメッセージを変更する場合は、templateに直接埋め込まず、現状の {{ errorMsg }} で問題ないかと思います^^

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 25, 2019

Author Owner

ありがとうございます。
動的にエラーメッセージを変更する方法で実装しました。

@@ -0,0 +1,68 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

client/test/unit/coverage に関するファイルが文字検索で確認したところ14ファイルほど追加されたようですが、これらは何かしらのツールを使って自動生成されたファイルでしょうか?
(自動生成されたファイルであれば、レビュー対象外のファイルとして判断)


自動生成されたファイルであれば、.gitignoreでバージョン管理から外した方が良いかなと思いますが、他の一般的なvueプロジェクトではバージョン管理に含めているでしょうか?
(テストを実行するたびにに更新が入ると、毎回レビュー不要なコードが大量に含まれることになるので修正差分に含まれないようにしたほうが良いかなと思いました。(package.jsonの内容をもとにライブラリを依存ライブラリをインストールしてnode_modulesを生成するように、コマンドで同じような自動生成ファイルが作られるのであれば、含める必要は無いかなと考えております。))

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 25, 2019

Author Owner

すいません。
自動生成されたファイルで必要ありませんでした。
.gitignoreでは指定されていたのですが、誤ってpushしてしまいました。
この場合は、コミットを取り消した方が良いでしょうか?

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

一度コミットした場合は、バージョン管理され続けるので、一度 git rm コマンドを使って、バージョン管理から削除してあげてから、.gitignoreで除外するファイル・ディレクトリのルールを追記すれば良いかなと思います^^

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 26, 2019

Author Owner

ありがとうございます。
git rmを使用してcoverageファイルを管理から外しました。


}))

describe('store actions.js', () => {

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

FYI

書き方の好みになるので修正の必要はありません^^


以下のようにdescribeで意味のある単位でまとめると、テスト実行結果の出力の見出しが段落ごとにまとめられて、実行結果が読みやすくなります。(describeを入れ子にすることで、実行結果の見出しも入れ子になり、itで意味のある単位で箇条書きがされるようになる)

コード内容

describe('store actions.js', () => {

  describe('The fetchTodos method', () => {
    it('success test', async () => {
      const commit = jest.fn();
      await actions.fetchTodos({ commit })

      expect(url).toBe('http://localhost:3000/api/todos')
      expect(commit).toHaveBeenCalledWith('getTodos', 'value')
    })

    it('catches an error', async () => {
      mockError = true
      await expect(actions.fetchTodos({ commit: jest.fn() }, {}))
        .rejects.toThrow('API Error occurred')
    })
  });

  describe('The postTodo method', () => {
    it('success test', async () => {
      mockError = false
      const commit = jest.fn();
      const newTitle = 'testTitle';
      const newBody = 'testBody';
      await actions.postTodo({ commit }, { newTitle, newBody })

      expect(url).toBe('http://localhost:3000/api/todos')
      expect(body).toEqual({ 'title': 'testTitle', 'body': 'testBody' })
      expect(commit).toHaveBeenCalledWith('addTodo', undefined)
    })

    it('catches an error', async () => {
      mockError = true
      await expect(actions.postTodo({ commit: jest.fn() }, {}))
        .rejects.toThrow('API Error occurred')
    })
  })

})

上記コードの実行結果画像

  • store actions.jsが大見出しとなり
  • The fetchTodos methodThe postTodo methodが中見出し
  • 各中見出しの中に、各メソッドごとのテスト内容が箇条書きで書かれる

image

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 25, 2019

Author Owner

ありがとうございます。
そちらの方が見やすいと思ったので、参考にさせていただき修正しました。

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Apr 25, 2019

他のmutationsなどのテストは、変更差分が大きくなるかと思ったので、今回は行わずレビュー依頼させていただきました。

了解しました^^
テスト実装差分だけを含めるブランチを生成してテストの実装をすれば良いかなと思います^^

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Apr 25, 2019

いくつかコメントしたのでご確認お願いします^^

}
},
methods: {
...mapActions([
'postTodo'
]),
postTodoButton() {
if (this.newTitle === '' || this.newBody === '') {
if (this.newTitle === '' && this.newBody === '') {

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

FYI
(修正の必要はありません^^)

空文字はfalse扱いになるので、以下のように短く書くこともできます^^

if (!this.newTitle && !this.newBody)

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 26, 2019

Author Owner

ありがとうございます。
短く書いた方が、良いと思ったので、修正しました。

@@ -19,8 +19,8 @@ describe('Textarea.vue', () => {
})
it('actions test', () => {
const wrapper = shallowMount(NewTodo, {store, localVue})
wrapper.find('[new-title]').setValue('testTitle')
wrapper.find('[new-body]').setValue('testBody')
wrapper.find('input').setValue('testTitle')

This comment has been minimized.

Copy link
@tsuyopon-xyz

tsuyopon-xyz Apr 25, 2019

FYI

仮にinputやtextarea要素が複数あったときに、意図しない挙動をするかもしれないので、より具体的にidセレクタやclassセレクタで指定できれば、input, textarea要素があっても、意図した要素を取得できるので、可能であれば限定してあげたほうが良いかなと思います^^
(CSSで指定したID値やclass値を持つ要素のスタイルのみ変更するのと同じイメージ)

実装イメージは以下のようなイメージです。
(クラスセレクタが指定できる場合)

wrapper.find('.new-title').setValue('testTitle')

This comment has been minimized.

Copy link
@keisuke3

keisuke3 Apr 26, 2019

Author Owner

ありがとうございます。
こちらも最初に試した時は指定できなかったのですが、試してみると指定する事ができたので修正しました。誤字チェックなどもっと気をつけるようにします。

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Apr 25, 2019

質問への返答・新たにコメントしたのでご確認お願いします^^

#10 (comment)

@tsuyopon-xyz

This comment has been minimized.

Copy link

tsuyopon-xyz commented Apr 26, 2019

👍

OKです^^
マージしても構いません^^

@keisuke3 keisuke3 merged commit 66c3d66 into develop Apr 26, 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.