OSSに初のプルリクを出そうと頑張ってみた

# 記事紹介
しっかりプロジェクトとしてコードを書くような経験がなく、なんでもいいからとりあえずチーム開発の流れを把握したいということでチャレンジ。
結果としては流れはわかったが、手を出したIssue自体は局所的にちょこっと直せば終わりという感じではなく、うまく直せているのかわからない。放置されている。忙しくてみられていないのかもしれないし、愛想を尽かされて見る気にもなれないのかもしれない。

**注意点**
- 慣れてる人からすると特に読む意味がない記事だと思います
- まだやったことない人からしても雑多過ぎてあまり読む気になれない記事だと思います
- 自分でももう少しまとめたり記事を分けたりしたいと思い続けていたが、それだといつまで経っても整理できないのでとりあえず長文で雑多に書く。自分でもまるっと読む気にはなれなさそう


# どのIssueなら手を出せそうか + 初期調査

AmplifyでGood First IssueラベルがついているIssueから探してみた。

プルリクは[レポジトリのContributingガイドライン](https://github.com/aws-amplify/amplify-backend/blob/0d8f5b36003efd69415a4feadc4734997d4b0fe7/CONTRIBUTING.md)に従えば出せそうなので、Issueを探す。
類似Issueがあるらしい以下のIssueをみてみた。

https://github.com/aws-amplify/amplify-backend/issues/825

`npm create amplify`コマンド実行中にCtrl + Cを押すとエラーが表示されてしまうのを抑制したいという内容で、[別のコマンドで同様の事象に対する修正](https://github.com/aws-amplify/amplify-backend/pull/865)がすでに行われていたためそれを真似れば良いものと考えた。

Ctrl + CはSIGINTを検知すれば良さそうである。

https://nandakagoodvibes.hatenablog.com/entry/2021/12/12/160950

今回の対象のファイルはこれ。

https://github.com/aws-amplify/amplify-backend/blob/main/packages/create-amplify/src/create_amplify.ts

# 類似Issueを確認
類似のissueである[top-level error handling](https://github.com/aws-amplify/amplify-backend/pull/865)をみてみる。これは`ampx sandbox`コマンドを対象にした修正である。
`yargs`というもののエラーハンドラーが正しくなかったとのこと。コミットを見てみると`.fail`というものが書き換えられており、yargsの正体を知らないといけない(yargsについては後述)。
`.fail()`でエラーを拾うのではなく、`process.on()`でエラー検知するリスナーをアタッチするように変えたようである。

<details><summary>参考: SIGINTを拾うコード例</summary>

`ampx sandbox`コマンドの実行中にCtrl + Cを押した場合は[以下の箇所](https://github.com/aws-amplify/amplify-backend/blob/53d5d1d33a440a1848852cbe77499dafc6ac598a/packages/cli/src/commands/sandbox/sandbox_command.ts)で検知され、`Would you like to delete all the resources ~~ ?`とプロンプトが出る

```ts
process.once('SIGINT', () => void this.sigIntHandler());
...
  sigIntHandler = async () => {
...
    const answer = await AmplifyPrompter.yesOrNo({
      message:
        'Would you like to delete all the resources in your sandbox environment (This cannot be undone)?',
      defaultValue: false,
    });
```
</details>

より具体的に、何を直したのかをみていく。

`ampx`コマンドのエントリポイント(実行コマンドの実態)は[package.json](https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/package.json)のこの部分でわかる。
```ts
  "bin": {
    "ampx": "lib/ampx.js",
    "amplify": "lib/ampx.js"
  },
```

`lib/ampx.js`ファイルはなかったが、おそらく`ampx.ts`をコンパイルすると出来上がるのだと思うので[ampx.ts](https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/src/ampx.ts)がエントリポイント。

**修正直後の状況**
修正直後はhandlerを定義する別ファイルを作成せず、`amplify.ts`というファイルに直接`process.on()`のリスナーを定義していたようだ。

https://github.com/aws-amplify/amplify-backend/blob/53d5d1d33a440a1848852cbe77499dafc6ac598a/packages/cli/src/amplify.ts

その後しばらくしてから`amplify.ts`から`ampx.ts`へ置き換えられたようだ。

https://github.com/aws-amplify/amplify-backend/pull/1407

いつ書き換えられたか？
`error_handler.ts`というファイルが作られる前、全てを`amplify.ts`に記述しているときの`amplify.ts`ファイルから変更があった[コミット](https://github.com/aws-amplify/amplify-backend/commit/255cc2c59ccb640746086fa9aba191be2aaf24db)と[Pull Request](https://github.com/aws-amplify/amplify-backend/pull/1356)。

**error_handler.tsを作成した後の状況**
最終的に、現在は`ampx.ts`で`attachUnhandledExceptionListeners`によるイベントリスナーのアタッチをしているし、エラー時は`errorHandler`を呼び出している。

https://github.com/aws-amplify/amplify-backend/blob/7962bc505254c1e624f8ce9abfb4a1da0cceb8ae/packages/cli/src/ampx.ts
```ts
attachUnhandledExceptionListeners(usageDataEmitter);
...
const parser = createMainParser(libraryVersion, noticesRenderer);
const errorHandler = generateCommandFailureHandler(parser, usageDataEmitter);
...
catch (e) {
  if (e instanceof Error) {
    await noticesRenderer.tryFindAndPrintApplicableNotices({
      event: 'postCommand',
      error: e,
    });
    await errorHandler(format.error(e), e);
  }
```

`attachUnhandledExceptionListeners`, `generateCommandFailureHandler`はいずれも[error_handler.ts](https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/src/error_handler.ts)で定義されている。

# 修正チャレンジ
## 直してみる(1回目)


類似Issueで追加されたリスナーとハンドラーは以下のように引数を取る。



https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/src/error_handler.ts

```ts
export const attachUnhandledExceptionListeners = (
  usageDataEmitter: UsageDataEmitter,
): void => {
```

```ts
export const generateCommandFailureHandler = (
  parser: Argv,
  usageDataEmitter?: UsageDataEmitter,
): ((message: string, error: Error) => Promise<void>) => {
```

`ampx`コマンドのサブコマンドとして実行されるsandbox(`ampx sandbox`として実行)と`npm create amplify`コマンドで実行されるcreate-amplifyは違うので、create-amplifyのエラーハンドラーでは引数はいらないと思っていた。

なぜなら、`UsageDataEmitter`はどのコマンドが成功したとか失敗したというメトリクスを排出してそうなことと(AWSサービス自体が収集している？)、`parser`は`yargs`のインスタンスであるため、サブコマンドを伴う`ampx`では使うが`create-amplify`では必要がないはずだからである。

ということで、とりあえずCtrl + Cを押してもエラーが出なくなるという動作を確認して1回目提出してみた。この時は引数を消した`error_handler.ts`ファイルを`create-amplify`用に作成(sandbox用のやつのほぼ複製)してみた。
その後修正したので変わってしまっているが、コミットとしては[ここ](https://github.com/aws-amplify/amplify-backend/pull/2364/commits/7e62d5f7e2d34bcefd9416e1876d58501bb51a48)が該当する。

## 返信
要は全然ダメで話にならん、という内容の指摘をいただいた。以下を言われた。
- cliディレクトリに似たようなエラーハンドラーのファイルを複製するのではなく、sandboxと共有しているcli-coreディレクトリに共通部分として抜き出してくれ
- `package-lock.json`が変わっているので変えないでくれ

類似Issueで追加されたエラーハンドラーでは`parser`が必須になっているのでそれを綺麗に書き換えるのが自信なく、元々あるファイルはいじらないようにしようとしたのだが...。やってみるしかない。

ディレクトリはcliでもcli-coreでも似たような構成になっているのでただ写すくらいの意識で良さそう。
[extractSubCommands(parser)はparserがundefinedの場合も考慮されていそう](https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/src/error_handler.ts#L72)なので、単に引数を`parser`ではなく`parser?`にするだけでいけそうだ。

## 2回目行った変更まとめ

[2回目の変更はこのコミット](https://github.com/aws-amplify/amplify-backend/pull/2364/commits/dcaa3ae7be6ffd97ed58aa83c25bf31d0612fb26)にまとまっている。
やったことはざっくりと以下。言うのは易しだが、もうすでに自信ない。

- `error_handler.ts`と`error_handler.test.ts`(+読み込んでいたメソッド)をcli-coreに移動
- `packages/cli-core/src/index.ts`に移動したファイルの`export`を記述。これにより`@aws-amplify/cli-core`の名前空間で読み込める
```ts
export * from './error_handler.js';
export * from './extract_sub_commands.js';
```

- `error_handler.ts`を`import`していたファイルの`import`方法を変える

- `package-lock.json`が変わってしまっていたのをなんとか変更なしの状態に修正

# 現在までの簡単なまとめ
これがPR.
https://github.com/aws-amplify/amplify-backend/pull/2364

- 1回目の変更
    - 類似のissue [top-level error handling](https://github.com/aws-amplify/amplify-backend/pull/865) があったので、それと同じようにエラーハンドリングを`unhandledRejection`を扱えるように修正。processにエラーハンドリングのためのリスナーをセットする必要があった。
    - コード自体は見よう見真似。何を治せばいいかの練習のつもり

- 返信来るまで
    - ずっと動きがなかったが、マージの仕方が悪く次のissueを探そうとして環境を整えた時にPRがクローズしてしまった。
とりあえずやる、でやったのでMainブランチでやっており、他の変更が加えられなくなった。
    - 別ブランチを作って同じ修正で再度PRを出したところ、そちらには反応があり全然ダメというコメント

- 2回目の変更
    - 共通の処理とは言いながら分岐が増えるので、どのように分けるか模索しながら変更した
また、不要なコミット(`package-lock.json`の変更)が意図せず入ってしまったところも修正に苦労した。

- 現在
    - 放置中。正直そこまで被害がないバグだし、気づかれていないのか愛想を尽かされているのかはわからない。

# 躓いたところや知ったこと
やったことの概要は以上で終わりだが、上記をやるのに色々知ったことがあり、それをメモるのが今回の趣旨。

## yargs

引数のパーサー。cliを作るなら必須と言っていいくらい便利らしい

https://github.com/yargs/yargs/blob/main/docs/api.md

typescriptの一番最初のサンプルはこんな感じ。以下で理由が書いてあるが、typescriptでargvの値を使いたい時は`.parse()`じゃなくて`.parseSync()`を使うと解決できる

https://github.com/yargs/yargs/blob/main/docs/typescript.md

parseの意味がよくわからなかったが、以下は同じものを表すとのこと。
https://github.com/yargs/yargs/blob/main/docs/api.md

```ts
require('yargs/yargs')([ '-x', '1', '-y', '2' ]).parse()
```

```ts
require('yargs/yargs')().parse([ '-x', '1', '-y', '2' ])
```

なお、`const argv = yargs(process.argv.slice(2)).parse([ '-x', '1', '-y', '2' ])`というコードを`ts-node index.ts -a 11 -b 22`と引数指定して実行した場合は、parseの方が勝ってユーザーのコマンドライン引数a, bは無視される(`x=1, y=2`でハードコードされた固定値になる)。


`fail`は引数指定をミスった場合に発生する。例えば以下でbは必須になっているので、実行時に-bを指定しなかった場合に`.fail`が発生

https://github.com/yargs/yargs/blob/main/docs/api.md#fail

```ts
import yargs from 'yargs/yargs';

const argv = yargs(process.argv.slice(2)).options({
  a: { type: 'boolean', default: false },
  b: { type: 'string', demandOption: true },
})
.fail(function (msg, err, yargs) {
  if (err) throw err // preserve stack
  console.error('You broke it!')
  console.error(msg)
  process.exit(1)
})
.parseSync()
```

<details><summary>`ampx.ts`で使われている`parser`はyargs</summary>

ampx.tsで使われているparserは[このメソッド](https://github.com/aws-amplify/amplify-backend/blob/0d8f5b36003efd69415a4feadc4734997d4b0fe7/packages/cli/src/main_parser_factory.ts)で作られている。



yargsの`.command`で事前に[createSandboxCommand](https://github.com/aws-amplify/amplify-backend/blob/0d8f5b36003efd69415a4feadc4734997d4b0fe7/packages/cli/src/commands/sandbox/sandbox_command_factory.ts#L31)、最終的に`sandboxCommand`(`sandbox_command.ts`からexport)が渡されているため、引数をparseして`sandbox`と一致すれば`sandbox_command.ts`で定義したクラスのhandlerが実行される。
これにより`ampx sandbox`が実行されたときに`sandbox_command.ts`の処理を行うよう、サブコマンドにより分岐できる。

yargsにcommandとかを渡すとhandlerが実行されてくれるというのはこのあたりかしら。
https://github.com/yargs/yargs/blob/main/docs/advanced.md#command-modules
```
exports.handler: a function which will be passed the parsed argv.
```

</details>

## node:test

v18から追加されたNode.js標準のTest Runner

https://nodejs.org/api/test.html


以下のようにすると`mockMethod1.mock.callCount()`で呼び出された回数がわかるようだ。どのルートを通ったかカウント1や0で数えるパターン
```ts
const mockMethod1 = mock.method(testClass, testMethod1);
```

## ローカルの変更が実行時に反映されない？
`npx create-amplify`コマンドと`npm create amplify`は「`create-amplify`パッケージの`package.json`のbinにあるファイルを実行する」という点は共通している。
ただし、前者はローカルの反映がされたが後者はされなかった。


`npm create amplify`だとどうしても外部からインストールしてしまうっぽい。`npm create`はローカルシステムディレクトリを見ないので`npm link`を考慮してくれない動作になっているとのことだが、生成AIに聞いただけで出典は見つけられていない。
`npx create-amplify` でやっとローカルの変更を反映して実行できた。

その他細かな知ったこと。
- `npx`は`npm install`して実行して`npm uninstall`してくれるようなもの

- `npm run watch`で差分コンパイルしてくれてとても便利
- `npm create amplify`は`npm create`を利用している
`npm`は`create-amplify`パッケージを探す。そのパッケージの`package.json`のbinに指定されているファイルがエントリポイントになる。
- `npm create`は`npm init`のエイリアス。`npm init foo` -> `npm exec create-foo`　という変換をされて、`create-foo`をレジストリから探して実行する。エントリポイントは当然`package.json`のbinになる。

https://docs.npmjs.com/cli/v10/commands/npm-init

- VSCodeのJavaScript debug terminal(コマンドパレットからも開ける)から`npx create-amplify`を実行すればデバッガもアタッチできる

## Git関連
### 過去のコミットのCommiterとAuthorだけ書き換える

**背景**
ユーザー名とemailを`git config`で登録しないと、コミット時の`XXXX committed 日時`のメッセージのユーザー名XXXXの部分がGitHubアカウントとリンクしなかった。その状態でコミットしてしまったので、それを書き換えたかった。

**手順**
1. `git rebase -i HEAD~n` を実行する。nは変更したいコミット数。

2. テキストエディタが開かれたら、変更したいコミットの行を見つける。その行の `pick` を `edit` に変更する。

3. `:wq` で保存。

4. 変更したいコミットに移動する。
   ```
   git checkout <commit-hash>
   ```

5. 新しいユーザー名を設定する。

   ```
   git commit --amend --author="New Name <new@email.com>"
   ```

6. `git rebase --continue` を実行して、リベースを続行する。

7. 必要に応じて、他のコミットも変更する。

8. 最後に `git push --force-with-lease` を実行してリモートに変更を反映させる。

commiterも変えるには`git commit --amend`
※ここでAuthorを変えても変更されないようで、commiterとは別に↑はやる必要がありそう。


### 一般的なContributeの作法
簡単なまとめに以下のようにしれっと書いたが、cloneしたレポジトリのmainレポジトリで作業をしてしまったのはやはり悪そう。

> ずっと動かなかったが、マージの仕方が悪く次のissueを探そうとして環境を整えた時にPRがクローズしてしまった。
とりあえずやる、でやったのでMainブランチでやっており、他の変更が加えられなくなった。

upstreamの変化を取り入れたい場合にfetchすると自分の加えた変更が消えてしまう(変更を消すとPRも自動で消えた)。

AmplifyのContributingガイドラインには書いていなかったが、上記に気づいたときに[初めてOSSのコントリビュートをした話](https://zenn.dev/kyuki/articles/3e32c89e3659c6)というありがたい記事を発見した。

ざっくり、以下のようなことを都度やるのが一般的であるようだ。
しばらく時間が経った後や、PRを出す直前に以下で最新化する。

```sh
git remote add upstream フォーク元リポジトリのURL
git fetch upstream
git checkout 作業ブランチ
git rebase upstream/main
```

※ 久しぶりにブランチ切るぞ〜という場合にはmainブランチで上記を行なうとmainブランチが最新化できるので、その後にブランチを切れば良さそうである。または、ブラウザでfork syncボタンを押しても同様のことになりそう。


変更後は以下でpush
```sh
git push --force-with-lease origin <branch_name>
```

不安なようなら以下で新しいブランチを作ってからマージという手も。
```sh
git checkout -b new_branch_name
git push origin new_branch_name
```

※Amplify Gen2の場合は事前に開発環境構築を行っている必要があったので、上記だけを行ってもpushできなかった(`package-lock.json`のconflictを`npm i`で解消した後のpushでエラーが出る

### `package-lock.json`が変更されてしまったのを戻す方法
おそらく先述のfetchをしなかった影響で(？)`package-lock.json`が変更されてしまい、「そのファイルは変更しないでくれ」と言われてしまった。俺も別に変更したくてしたわけではない。
が、ここを変えるのにgit慣れが足りず苦労した。

以下のような順序のコミット履歴になってしまっていた。
1. 不要な`package-lock.json`の変更を含むコミットがある(1回目のPR)
2. ごちゃごちゃやって、`package-lock.json`を消すだけのコミットをしてしまう

特定のコミットの中から`package-lock.json`の変更だけを取り消したい。

以下の記事をコマンド参考として準備した。

[あのコミットをなかった事に。git rebase -i の使い方](https://www.karakaram.com/git-rebase-i-usage/)

[commitに含まれる特定ファイルのみをcommitから外す](https://qiita.com/dev_tatsu/items/f853cdff5ffb547e7047)


新しいコミットから順に直していくことでできた。
自分の場合は上記の2のコミットを消し、1のコミットを修正するという形になった。

### 別のブランチからの強制マージ

1回目の変更を行ったブランチAでPull Requestを出しておりそちらは完成まで汚したくなかったので、ブランチBを作って作業を行った。
最終的には追加修正をブランチAにマージする必要があるが、上記の通りブランチBでrebaseなど結構色々行ったためコンフリクトがすごかった。
ブランチAは全然ダメと言われた全く消したい内容なので、以下のようにしてブランチBを正として強制的にマージした。

```sh
git checkout ブランチA
git reset --hard ブランチB
git push  -f  # リモートのブランチAにブランチBの内容が反映される
```