Skip to content

GitHub Flow Simulation app #39

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

Closed
wants to merge 24 commits into from
Closed

Conversation

rie-oki-agn
Copy link
Member

@rie-oki-agn rie-oki-agn commented Jun 28, 2022

GitHubフローのシミュレーションページ作成中。
netlifyプレビューのためのPRです。
開発の議論やアドバイス、このコマンドを追加してほしい!などありましたらコメントお願いします!!

@netlify
Copy link

netlify bot commented Jun 28, 2022

Deploy Preview for commit-mate-net ready!

Name Link
🔨 Latest commit 5c61fb9
🔍 Latest deploy log https://app.netlify.com/sites/commit-mate-net/deploys/62beca0104ab0a0008dafdbc
😎 Deploy Preview https://deploy-preview-39--commit-mate-net.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@rie-oki-agn
Copy link
Member Author

/tutorial

Copy link
Member Author

@rie-oki-agn rie-oki-agn left a comment

Choose a reason for hiding this comment

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

vue3のバグ?

46行目〜
toRawをして参照を外すことには成功、値を変更してもやっぱり参照されてないので元のref()の方は変化なし。
なのに、全く別のref()の値へ、そのtoRawした値をpushした途端、toRawの効果が切れて元のようにまた参照し始める(pushした後、変更が連動している)

その証拠に、toRaw化してrefを外したオブジェクトのnameプロパティの冒頭にorigin/追加(追加した時点で何も変化なし)、そして別のref()の配列に、nameプロパティ: 'test'、commitsプロパティはそのtoRaw化した値を注入したオブジェクトをpushすると、pushされた方は正しく表示され、toRaw化してた方のrefの名前は'origin/'が追加されてた。そしてそのtoRaw化した方の元のrefのcommitsが変更するたび、pushしたさきのcommitsも連動し始めた

@monsat
Copy link
Member

monsat commented Jun 28, 2022

なにかしらやろうとしていることが違っている気がする
やろうとしていることは toRaw(local) だと思うけど、多分、それではなさそうな第一印象

配列をコピーしたい感じ?

@rie-oki-agn
Copy link
Member Author

rie-oki-agn commented Jun 28, 2022

↑の commit でソースコードの解説コメント追加しましたー!

やりたいこと:現時点での、あるブランチの状況(オブジェクトである)(プロパティにはブランチ名、現時点でのコミットの数がある)をスナップショット(toRaw())して、その静的なオブジェクトデータをoriginの配列へarray.push()したい(コピペしたい)
(→その後array.push()がいけなかったのかな?と思い、originを[ ] ではなく{ } に変更し、array.push() ではなく{}を追加するような処理に書き換えたが、同じ問題が発生....)

問題:toRaw()したはずなのに、push()した途端、ref()に戻ってしまう→ local と origin のコミットの数が連動してしまう...。push()した後に再度、localでコミット2,3,4,5....とするとoriginのコミットも同じように2,3,4,5....と増えてしまう。

公式による「永久的には保持されない」とはこのこと...?(日本語が理解できない...)
一時的に読み込むだけなのか..
スクリーンショット 2022-06-28 21 58 33

@rie-oki-agn
Copy link
Member Author

rie-oki-agn commented Jun 28, 2022

.length することで、refから解放されそう(解決しそう)です
commits[]を分解し、一つ一つ値(commit id)をとり出して新たな配列データとすることでrefから解放されそう
もっと簡単に参照をやめることってできないのでしょうか..

@rie-oki-agn
Copy link
Member Author

commit id たちが格納されている配列を、参照から解放するために、新たな箱を作ってforeachで入れ直したら、とりあえず解放されうまく行きました!

@wako-p
Copy link
Contributor

wako-p commented Jun 28, 2022

おそらく、配列の参照問題なのかなって思いました。
また、配列はpush()などのミュータブルで破壊的な操作よりもconcat()のように新しくインスタンスを生成して返してくれるイミュータブルな操作を心掛けると良いかもです!

// 引数なしで呼ぶと、同じ内容の新しい配列がシャローコピーされる
targetBranch.value.commits = snapBranch.value.commits.concat()

↑こんな感じ
そうすると、今回のような参照問題も必然的に回避できるはずです

それから、同じ理由で配列の内容を加工したいなーって場合には
map()とかreduce()とかを使ってあげると良いと思います。

もしくは、プリミティブな配列の使用をやめて
immutable.js のようなイミュータブルなデータ構造を提供するライブラリを使うとか?

全然、検討違いなこと言ってたらすみません、、、

@monsat
Copy link
Member

monsat commented Jun 29, 2022

ちゃんとソースみました!

@rie-oki-agn
toRaw() は、リアクティブなオブジェクトを、通常の(リアクティブではない)オブジェクトに変換する関数なので、今回の使用目的ではないのではないかと想像しています。

配列やオブジェクトのコピーは基本的に下記のようにすると読みやすくなります。
(forEach とかもいらなくなります)

const copiedArticles = [ ...articles ]
const copiedUsers = { ...user }

たとえば次のようにすることで name を上書きするみたいなこともできますね

const newUser = {
  ...user,
  name: 'new_name',
}

リポジトリ相当については、リモートとローカルで、配列もしくはオブジェクトは統一してよさそう

一旦できるところまで実装してくれたら、木曜日か土曜日にそれをもとに補足しますね
まずは動くところを目指しましょう。そのうえで、リファクタリングしつつ、よりメンテナンスしやすい書き方になるようサポートします。

@wako-p
Copy link
Contributor

wako-p commented Jun 29, 2022

@monsat

const copiedArticles = [ ...articles ]
const copiedUsers = { ...user }

↑これ、スプレッド構文って言うんですね!
たしかにconcat()は本来の目的とは違う使われ方なのでこっちの方が適切ですねー

ただ、スプレッド構文もシャローコピーっぽいので
ネストが深い場合は気を付けないとですね!

@monsat
Copy link
Member

monsat commented Jun 29, 2022

いちおう補足でシャローコピーとディープコピーについて

JavaScript によるオブジェクトや配列のコピーは基本的にシャローコピー※で、中にオブジェクト(or配列)が入っていると、それはコピーされません
コピーされないということは、参照を保持するということです

const user = {
  name: 'コピーされる',
  age: 25, // コピーされる
  followIDs: [ 123, 234, 789 ], // 配列はコピーではなく参照が渡される = 変更したらコピー後のオブジェクトも変更される
}

これを簡易的にディープコピーするとしたら

const copied = {
   ...user,
   followIDs: [ ...user.followIDs ],
}

というようにする必要があります(他にも配列やオブジェクトがあればその分だけ必要で、配列の中にオブジェクトとかあったらそれらもすべて必要)

このようにしておくと、user.followIDs に push しても copied.followIDs には影響がありません。

※ ディープコピーをするならlodash等を使うのが一般的でですが、慣れるとディープコピー不要な設計が可能になります

注記
※ (オブジェクトを const しても、プロパティの値は書き換えられるのも同様ですね const foo = { name: 'foo' }; foo.name = 'bar'

@rie-oki-agn
Copy link
Member Author

rie-oki-agn commented Jun 29, 2022

@wako-p @monsat

なんと!Vueのリアクティブ云々ではなくて、JavaScriptの参照の問題が絡んでいたのですね!!
concat()の本来の使い方も、スプレッド構文の活用方法も、勉強になりました。ありがとうございます!
スプレッド構文の、展開した時に被ったプロパティを上書きしてくれる(?)のはすごい便利ですね!

調べているときにdeepという単語が出てきたのですが、そういうことだったのか....!!
ネストをコピーする場合JSでは基本的に一番浅い階層しかコピーが作成されないということですね!理解しました!スッキリ!

@monsat

pushの段階では全然考えてなかったのですが、確かに
pullする時は「origin -> local (ターゲット)」となるので
localの方もオブジェクトにして、統一しようと思います!

土曜日にレビューお願い致します!
ディープコピー不要な設計も勉強して共有したいですね。

@wako-p
Copy link
Contributor

wako-p commented Jun 29, 2022

@rie-oki-agn @monsat
ディープコピー不要な設計っていうのは私も気になりました!
機会があれば、是非!!

@rie-oki-agn rie-oki-agn changed the title Tutorial demo GitHub Flow Tutorial app Jun 29, 2022
@rie-oki-agn rie-oki-agn changed the title GitHub Flow Tutorial app GitHub Flow Simulation app Jun 29, 2022
@rie-oki-agn
Copy link
Member Author

rie-oki-agn commented Jul 2, 2022

この会話は #46 へ、作業は46-simulationへ移します

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.

3 participants