Skip to content

Training0 - #1

Open
ookura-mf wants to merge 5 commits into
mainfrom
training0
Open

Training0#1
ookura-mf wants to merge 5 commits into
mainfrom
training0

Conversation

@ookura-mf

Copy link
Copy Markdown
Owner

No description provided.

Comment thread .node-version
@@ -1 +1 @@
nodejs 16.13.0
16.13.0

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

前PRから引き継ぎ

ookura-mf 1 hour ago
nodejsが入ってるとnodejsなんて知らん!ってerror出たので修正したが、実はそのままでいけるのかわかっていない(nodenvのrepositoryでgrepした感じもヒットしなかった)

Owner
@Y4suyuki Y4suyuki 26 minutes ago
もしかしたら僕の用意したのが間違っていたかもです 😅
nodeenv 使ったことなくて、今も使っていません
nodeのバージョン管理だとnvmの方が主流な気がします(スターの数とかブログとかで取り上げられる頻度)
僕はasdfを使って .nvmrc を読むようにしてます

https://github.com/ekalinin/nodeenv
https://github.com/nvm-sh/nvm
https://github.com/asdf-vm/asdf

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvmの方が主流なんですね 👀
anyenvな気持ちですぐ使っちゃった 🙈

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

まあnodeenvもnode.jsの公式サイトに紹介されているしバージョンマネージャでもDockerでもnodeのバージョンさえあっていれば良いのでそこは個人の好みでも良いと思います(余程マイナーなものでなければ)

type GameHistory = {
squares: SquareValue[];
};
type GameProps = {};

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ookura-mf ookura-mf 1 hour ago
propsを取らない場合は空であることを明示しておいた方が良いのかな、というのとコンパイラがエラー吐くので定義してみたけどあっているかわからない

Owner
@Y4suyuki Y4suyuki 23 minutes ago
あっているかわからない

あっているとは何を指していますか?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あっているとは何を指していますか?

一般的な書き方(デファクト)なのかわからない、という意図でした!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一般的な書き方(デファクト)なのかわからない

なるほど、それで言うと一般的な書き方(デファクト)をそこまで意識する必要はないと思います
railsのconvention over configuration的な考え方はReact / Typescriptには当てはまらないと思います
conventionもあるにはあると思いますがまず、動くこと、一般的なProgrammingのgood practice (DRY, KISS, YAGNIなど)に当てはまっていることを考慮した上でそれでも揺らぎがある場合にチームでスタイルを統一すれば良いと思います
ここで言うとclass componentがpropsを取らないとしてもgenericsの型は設定しないといけないので迷うとしたら、名前をつけて定義するかgenericsのところにliteralで表記するかどちらかと思いますが、どちらでもコードとして大差ないのでどっちでもいいのではと思います(こう言うところの書き方にこだわって時間使う方が無駄だと思います)

]
},
"devDependencies": {
"prettier": "2.5.0"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Y4suyuki Y4suyuki 1 hour ago
👍

Owner
@Y4suyuki Y4suyuki 1 hour ago
prettierのconfigってどこにありますか?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Y4suyuki
普段これがいいってものも持ってないので全部defaultで使ってみようと思い特に用意してなかったです 🙈
が、とりあえず空ファイルだけでもおいときます

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

空ファイルを置くことが重要です!
prettierを使う場合Repoで設定を共有することが目的なので個人がプロジェクトのディレクトリに勝手においたり、個人のグローバルの設定を読みにいくのを防ぎます

あと設定についてはそもそもPrettierは(ESlintも個人的には)頑張って設定するものではないです
詳しくは
https://prettier.io/docs/en/option-philosophy.html

Comment thread tic-tac-toe-app/src/index.tsx Outdated
</button>
);
}
type SquareValue = string | null;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

より厳密な型定義にできないでしょうか?

https://www.typescriptlang.org/docs/handbook/2/everyday-types.html

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Y4suyuki
修正してみました!

@@ -0,0 +1 @@
{}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Y4suyuki Y4suyuki left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

お疲れ様でした!
training0へどうぞ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants