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

記事紹介

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

注意点

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

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

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

プルリクはレポジトリのContributingガイドラインに従えば出せそうなので、Issueを探す。
類似Issueがあるらしい以下のIssueをみてみた。

npm create amplifyコマンド実行中にCtrl + Cを押すとエラーが表示されてしまうのを抑制したいという内容で、別のコマンドで同様の事象に対する修正がすでに行われていたためそれを真似れば良いものと考えた。

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

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

類似Issueを確認

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

参考: SIGINTを拾うコード例

ampx sandboxコマンドの実行中にCtrl + Cを押した場合は以下の箇所で検知され、Would you like to delete all the resources ~~ ?とプロンプトが出る

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,
    });

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

ampxコマンドのエントリポイント(実行コマンドの実態)はpackage.jsonのこの部分でわかる。

  "bin": {
    "ampx": "lib/ampx.js",
    "amplify": "lib/ampx.js"
  },

lib/ampx.jsファイルはなかったが、おそらくampx.tsをコンパイルすると出来上がるのだと思うのでampx.tsがエントリポイント。

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

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

いつ書き換えられたか？
error_handler.tsというファイルが作られる前、全てをamplify.tsに記述しているときのamplify.tsファイルから変更があったコミットとPull Request。

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

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で定義されている。

修正チャレンジ

直してみる(1回目)

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

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

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用のやつのほぼ複製)してみた。
その後修正したので変わってしまっているが、コミットとしてはここが該当する。

返信

要は全然ダメで話にならん、という内容の指摘をいただいた。以下を言われた。

• cliディレクトリに似たようなエラーハンドラーのファイルを複製するのではなく、sandboxと共有しているcli-coreディレクトリに共通部分として抜き出してくれ
• package-lock.jsonが変わっているので変えないでくれ

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

ディレクトリはcliでもcli-coreでも似たような構成になっているのでただ写すくらいの意識で良さそう。
extractSubCommands(parser)はparserがundefinedの場合も考慮されていそうなので、単に引数をparserではなくparser?にするだけでいけそうだ。

2回目行った変更まとめ

2回目の変更はこのコミットにまとまっている。
やったことはざっくりと以下。言うのは易しだが、もうすでに自信ない。

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

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 があったので、それと同じようにエラーハンドリングをunhandledRejectionを扱えるように修正。processにエラーハンドリングのためのリスナーをセットする必要があった。
  • コード自体は見よう見真似。何を治せばいいかの練習のつもり

• 返信来るまで

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

• 2回目の変更

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

• 現在

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

躓いたところや知ったこと

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

yargs

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

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

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

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

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が発生

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()

`ampx.ts`で使われている`parser`はyargs

ampx.tsで使われているparserはこのメソッドで作られている。

yargsの.commandで事前にcreateSandboxCommand、最終的に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.

node:test

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

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

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になる。

• 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 <[e-mail]>"
   

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のコントリビュートをした話というありがたい記事を発見した。

ざっくり、以下のようなことを都度やるのが一般的であるようだ。

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

変更後は以下でpush

git push --force-with-lease origin <branch_name>

不安なようなら以下で新しいブランチを作ってからマージという手も。

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 の使い方

commitに含まれる特定ファイルのみをcommitから外す

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

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

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

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