React コンポーネントの改修で意図せず大域変数を参照してしまった事象の考察と対策
事象
以下のような React コンポーネントがあった。
interface SomeComponentProps { close: () => void; } export const SomeComponent: FC<SomeComponentProps> = ({ close }) => { return ( <> <Foo close={close} /> <Bar /> ... </> ); };
機能の改修によって Foo
に close
を渡す必要がなくなり、その結果 close
への参照がなくなったので props から close
を削除した。
export const SomeComponent: FC = () => { return ( <> <Foo /> <Bar /> ... </> ); };
ところが、参照がなくなったというのは思い込みで、実は他の箇所から参照されているのを見落としていた。
export const SomeComponent: FC = () => { return ( <> <Foo /> <Bar /> ... <Baz close={close} /> </> ); };
このような場合、大抵は未定義の変数を参照しようとしてエラーに気づくことができるだろう。ところが運の悪いことに、今回の事象では大域変数であるところの Window.close
を参照する形になってしまい、エラーにならず様々な目をすり抜けてしまった。また、TypeScript を用いたコードベースだが、当該箇所はシグネチャー的にも問題なく代入できてしまったために型のチェックもすり抜けてしまった。
Window.close
は window.close()
1 の形で呼び出すことが多いかもしれないが、単に close
で参照できてしまう。これは、Window がグローバルオブジェクトであり、そのプロパティがグローバルスコープに追加されているため。
https://developer.mozilla.org/en-US/docs/Glossary/Global_object
In a web browser, any code which the script doesn't specifically start up as a background task has a Window as its global object. ... The properties of the global object are automatically added to the global scope.
結果として、本来意図した「閉じる」挙動の代わりに、何も起きないかあるいは稀にブラウザのタブやウィンドウが閉じられてしまう事態になった2。
考察
これはどうすれば防げただろうか。また、繰り返さないためにはどうすればいいだろうか。
テストを書く
真っ先に思いつくべきはこれだろう。具体的に何をすべきかも自明だ。
そもそもテストがなかった理由については、当該コンポーネントが実装された当時とにかくスケジュールに追われており、実装者もレビュワーもテストへの意識が疎かになっていたであろうことが挙げられる。そして、技術的負債として認識されつつも工数を捻出する機会がなく今に至る。とはいえ、改修時に無いものは無いと認識してテストを追加するよう意識すべきだったという反省がある。
また、単純な機能ほどテストを書く意識が薄れてしまいがちだが、リグレッションを避ける意味での重要性を再確認するのには良い機会だった。
他にも色々と思うところはあるが本筋から逸れるので割愛。
大域変数への参照をエラーとする
テストを書くのはそれとして、不注意によるミスは生じうるもの。そこで、いっそのこと紛らわしい大域変数を参照できないようにしてしまえばいいのではないかという発想に至った。
その方針で調べたところ、TypeScript の issue の中で ESLint の no-restricted-globals
ルールを用いる解決策が提示されていた。これなら必要に応じて間違いやすいものだけを選んだり、document
や location
などよく使うものを除いて禁止するなど融通が利いていい。今回のような事象を未然に防ぐものとしては十分なので、早速採用することにした。
筆者の場合は特に何も除外せず // eslint-disable-next-line
などで部分的に例外を作るアプローチを採用した。理由は以下。
- 大域変数への参照が多くないこと
- 何を「間違いやすい」とするかは主観によるところが大きく、線引きが面倒
- 自転車置き場の議論になりかねない
- 不都合が生じてから見直せばいい
大域変数の名前と衝突しないように局所変数を命名する
ところで、React の props の命名において、イベントハンドラーは on
を接頭辞にするのが通例だ (e.g. onClose
)。客観的事実として、そのような命名規則を遵守させるためのルールが eslint-plugin-react に存在する (react/jsx-handler-names
)。問題となったコードベースにおいても基本的にはそのような命名が心がけられているのだが、あくまで指針レベルであり静的解析に組み込んだりはしていない。今回の事象に関してはこのルールを組み込んでいれば免れたかもしれない。
と、ここまでは props のプロパティ名に限った話。
今回のケースからは逸脱するが、根源を辿れば祖先にあたるコンポーネントの中で関数を定義しているはずで、その識別子の命名は props の命名とはまた別の話になる。とはいえ、これも react/jsx-handler-names
のルールの通り handle
を接頭辞にするのが通例で、当該ルールを組み込めば問題にならないだろう。
export const Parent: FC = () => { const handleClose = useCallback(() => { ... }, []); return <Child onClose={handleClose} />; };
ただし、グローバルスコープには onload
のように on
から始まるものも潜んでいるので油断はできない3。これらが小文字のみであるのに対して props は camelCase での命名が一般的ではあるが、人間である以上は不注意で取り違えることもあるだろう。ただし、props に限った話ではこれまた前掲の react/jsx-handler-names
が camelCase の命名を強制するようにできているため、これを導入すれば問題にならないかもしれない4。
今回の対策としては前述の no-restricted-globals
で事足りるし、修正すべき箇所が多いこともあって直近の導入は見送った。とはいえ、命名規則の観点においては指針が存在する以上は仕組みでカバーされる方が好ましいので、折を見て導入を検討したい。
感想
これまでも open
や close
を間違えて参照したことはあれど未然に気づけていたが、不注意が重なって今回の事態に至った。リスクと認識してはいただけに悔しいが、結果として学びから対策に繋げられたので良かったのではないか。
付録
環境及びバージョン等
さほどバージョンは重要でないはずだが一応。あくまで文中の記述の根拠として補助的に提示しているものであり、正確さを保証するものではない。
- React: 18
- TypeScript: 4.9
- ESLint: 8.34.0
eslint-plugin-react
: 7.32.2
- 念のため、Window と window は表記揺れではない。↩
-
Window.open
によって開かれた場合を除けば基本的には何も起きなかったはず。https://developer.mozilla.org/en-US/docs/Web/API/Window/close
↩This method can only be called on windows that were opened by a script using the Window.open() method.
-
その他のものについては TypeScript の lib.dom.d.ts に定義される
GlobalEventHandlers
が参考になるだろう。↩ - そこまで考慮して設計されているのではないかと思うが、実際のところは知らないし調べてもいない。↩