Lycolog

今回は実際の開発現場で遭遇しそうなコードパターンを2つ挙げ、それらを主に可読性向上と保守コスト削減の視点から、どのように改善できるかを具体的に書いてゆく。

サンプルコードはTypeScript + GraphQLを使ったAPサーバーを想定して書いている。

• 命名とデータ構造の最適化
  • 元コード
  • 改善後コード
  • まとめ
• 型安全で見通しの良いエラーハンドリング
  • 元コード
  • 改善後コード

命名とデータ構造の最適化

この項目ではエラーメッセージを返すサーバー側の処理について記述する。

元コード

resolvers/customer-info/helpers/invalid-reason.ts

type ReferencePageLinkMapKey = 'userFormIssue' | 'duplicateUserName' | 'someErrorMessage';

// exportされているがgetInvalidInfo(key, boolean)でreferencePageLinkMap.get(key)が返ってくることを確認するテストに使われているだけ
export const referencePageLinkMap: Map<ReferencePageLinkMapKey, string> = new Map([
  ['userFormIssue', 'https://help.example.com/23843ddvwa'],
  ['duplicateUserName', 'https://help.example.com/xdxdk2t5'],
  ['someErrorMessage', 'https://help.example.com/pp33s'],
  ...
]);

const invalidReason = {
  inputIssue: {
    message: '入力項目にエラーがあります。...',
    links: [
      {
        label: '【登録時のヘルプページをご確認ください】',
        url: referencePageLinkMap.get('userFormIssue') ?? '',
      },
    ],
  },
  ...
};

export const getInvalidInfo = ({
  errorReasonType,
  isModeA,
}: {
  errorReasonType: ErrorReasonType;
  isModeA: boolean;
}): Omit<SomeFormInvalidInfo, 'invalidReason'> => {
  switch (errorReasonType) {
    case 'INVALID_INPUT':
    case 'SOME_INPUT_ERROR':
      return invalidReason.inputIssue;
...
   };
}

このコードの問題点

• 定義する必要性が希薄な型情報があり、改修コストが高い
• 変数や関数名、キー名などから相関する機能の関連性を読み取りづらい
• 定義されたスコープを超えて利用されているもので命名から機能の読み取りが難しいものがある
• リテラルがべた書きされており将来のリファクタリングに支障がある
• 不必要にMapが使用されており、実装が冗長になっている
• 静的な値の取得にNull合体演算子が使用されており、潜在不具合となりやすい
• 型情報がべた書きされすぎており、見通しが悪い
• 複数のフラグ値の正規化を利用側で行っており、複雑になっている

改善後コード

リソースファイルと実装が一緒くたであると読みづらいためファイルを分割する。パスも書いているがパスについては深く考慮できていない。

resources/messages/CustomerError.ts

export const ERROR_MESSAGES = {
  formValidationError: {
    message: '入力項目にエラーがあります。...',
    helpLinks: [
      {
        label: '【登録時のヘルプページをご確認ください】',
        url: 'https://help.example.com/23843ddvwa',
      }
    ],
  },
  ...
} as const;

まずtype ReferencePageLinkMapKeyは無駄に型の管理コストが増えるだけなので削除する。意味がないとは言わないが、型の追加削除によって発生するデグレードは普通は型検査でわかるためなくてよいだろう。

次にreferencePageLinkMapでは意味がわからないのでERROR_MESSAGESにリネームする。中身のキーもinputIssueでは分かりづらいのでformValidationErrorとし、フォームのバリデーションエラーであることが分かるようにする。linksについても何のリンクなのかわからないためヘルプのリンクであることを明示できるようにリネームする。

helpLinks.urlについては、共通のURLを参照する概念は普通ないか、あったとしても別に切り出して管理するのはコストなので直に書いてよいと考える。元のコードでもテストコード以外には使われていない。テストコードには実値を書かないと回帰テストとして機能しないため、この方式のほうがより良いと考える。

例えば以下のようなテストコードは回帰テストの観点では意味が薄い。

it('INVALID_INPUTの時にurlが正しい結果になること', () => {
  const invalidInfo = getInvalidInfo('INVALID_INPUT', false);

  expect(invalidInfo.links[0].url).toBe(invalidReason.get('inputIssue'));
});

これは以下のように書くことでページのURLが変更されたときにテストが失敗するため、より価値のあるものになる。但しこれはexpect(invalidInfo).toStrictEquals({ ... })の形で一括判定したほうが、テストコードの可読性の観点からより良いだろう。

it('INVALID_INPUTの時にurlが正しい結果になること', () => {
  const invalidInfo = getInvalidInfo('INVALID_INPUT', false);

  expect(invalidInfo.links[0].url).toBe('https://help.example.com/23843ddvwa');
});

ERROR_MESSAGESをリソース変数としてみなせば、これはべた書きというより定数定義とみなせるため、基本的には問題ないと考える。勿論これは同じURLを持つものが大量にあるなど、ケースによっては考慮の余地はあるだろう。

またMapをやめ、敢えて直値を書くことで万一存在しなかった場合に、元にあった以下の空文字が返ってきて機能しなくなる問題も解決している。そもそも通る余地がないロジックなので存在しなくてよい。

url: referencePageLinkMap.get('userFormIssue') ?? ''

またERROR_MESSAGESにas constを付与することで、意図しない破壊が起きる可能性を減らすことができる。

handlers/error/GetCustomerInfoError.ts

export const getCustomerInfoError = (errorType: CustomerErrorType) => {
  switch (errorType) {
    case CustomerErrorType.INVALID_INPUT:
    case CustomerErrorType.SOME_INPUT_ERROR:
      return ERROR_MESSAGES.formValidationError;
...
   };
}

まずgetInvalidInfo()をgetCustomerInfoError()にリネームすることで意味を分かりやすくしている。次に複雑な型情報を除去してシンプル（= (errorType: CustomerErrorType) =>）にする。errorTypeはerrorReasonTypeとisModeAを合成した結果を渡すことを想定している。これによって、この関数内の複雑性を減らすことができる。またCustomerErrorTypeはENUMなので、case 'CODE1':のようにリテラルべた書きを回避できるうえ、仕様変更などで値が変わった時にも対応がしやすくなる。

また戻り値の型も削除する。これは静的解析により自明であるほか、Omit<SomeFormInvalidInfo, 'invalidReason'>とあるが、実際に返しているのはinvalidReasonであり、関連性がないためだ（おそらく、偶々よく似た型を転用しているのだろう）。またこの事例では更にinvalidReasonという変数名があり、Omitの内容と視覚的に競合し、コードの読解が難しいため、この修正により可読性が増す。

また改修前はエラーメッセージオブジェクトのキーと、ヘルプページのキー名、そしてswitchのキーの全てが食い違っており、見ていて混乱する内容だったが、ヘルプページのURLをべた書きするように変えたため、混乱する要素を減らせている。

まとめ

コードの記述量を減らし、長大なコードを別々のファイルに分けることや、過度に分割しすぎないことによって可読性が向上し、コード量が減ったため保守コストも削減できたと考えている。

半面、静的解析のコストは上がっているが、静的解析のコスト増よりも可読性がよく、保守コストが低いコードのほうが効率的な開発に寄与するだろう。

また静的な値を取得するためにMapを使い、TypeScriptを利用しているため、理論上undefinedが返ってこないのに、それを期待するロジックを削除することで、コードを読んだ人が混乱する可能性も削減している。

型安全で見通しの良いエラーハンドリング

この項目ではエラーメッセージを返すサーバー側のエラーハンドリングについて記述する。

元コード

try {
  // API呼び出しなど
} catch (error) {
  if (error instanceof ApolloError) {
    if (error.graphQLErrors[0]?.extensions?.ERROR_KBN === 'ERR_XYZ') {
      // エラー処理
    } else if ...
    ...
  }

  if (error instanceof SomeError) {
    ...
  }
  ...
}

このコードの問題点

• エラーハンドリングが長々とべた書きされていて見通しが悪い
• 配列の添え字が直に指定されているが意図が読み取れない
• 例外処理なのにOptional chainingが多く確実な例外処理に支障がある（例外処理で例外が発生するリスクは最小限にする必要がある）
• extensions配下の型情報が暗黙的でわからない

  graphQLErrors: ReadonlyArray<{
   extensions: {
       [attributeName: string]: unknown;
   }
  }>
  

改善後コード

const handleApolloError = (err: ApolloError) => {
  if (err instanceof ApolloError) {
    // graphQLErrorsが空であれば例外をスローし、そうでなければgraphQLErrorsを返す
    const gqlErrors = parseGQLErrors(err.graphQLErrors);
    // graphQLErrorsの中身をパースし、意味のある型を付けて返す
    const extensions = parseThisFunctionErrors(gqlErrors);
    if (extensions.ERROR_KBN === 'ERR_XYZ') {
      // エラー処理
    } else if ...
  }
}

try {
  // API呼び出しなど
} catch (error) {
  if (error instanceof ApolloError) {
    handleApolloError(error);
  } else if (error instanceof SomeError) {
    ...
  }
  ...
}

べた書きされているとコードの見通しが悪いため、catch句の中では例外種別ごとにハンドリングする処理に飛ばし、そっちで処理できるようにする。

error.graphQLErrorsは要素が0の場合があるため、parseGQLErrors()のような共通関数を作り、要素があれば中身を返す、なければ例外をスローして、より上位の処理に飛ばすなどの処理を共通的に行うようにする。この仕組みを共通化することで、この要素に対する処理の一貫性を持たせることができる。

またgraphQLErrorsの詳細については処理によって内容が異なるであろうことから、ドメインごとにparseThisFunctionErrors()のような関数を作り、その中で適宜データを整形するのが望ましいだろう。

そうして結果的に意味のある型情報を持ったextensions、あるいは適当な結果情報をハンドリングすることで、型安全かつ、責務が別れ、疎結合な実装に寄与する。

この形式であればhandleApolloError()は必要に応じて別ファイルに切り出し単体テストを書くこともできるし、このままでもあっても規模が小さければ十分テスト可能だろう。

なお、parseGQLErrors()やparseThisFunctionErrors()の具体的な内容については今回は省略する。

最近思っているNext.jsを使った画面設計に関する考えを箇条書きで雑に殴り書きしていく。この記事は考えの垂れ流しなので深い説明はしない。AppRouterではなく、PageRouterの考え。

• TypeScriptで実装し、型が騙せるような実装は極力避け、コードによる戻り値の型指定は不具合の原因になることがあるため、可能な限り型推論に任せる
• SOLIDな設計を意識することで疎結合でテストしやすい設計になる
• Clean Archtectureを意識することでSOLIDのSを意識しやすくなる
  • 画面として考える場合、実装レイヤーとしてはAPIを呼ぶ以外何もしないAdapter、ビジネスロジックやイベントハンドリングの実処理などを行うController、画面要素を配置しただけのView、画面状態を保持するState、それらをつなぎ合わせるUsecaseが、Usecaseを置くだけのPage（Next.jsのpageコンポーネントに埋め込むコンポーネント）、SSGやSSRをする場合のServer Side Controllerがあるとよいと考えている。大まかには下図のような感じで考えていて、過去の実務でもこれに相当するものを作ったことがある。
    

    

  • ただこれはModelに相当するものがなく、ビジネスロジックの共通化に課題が出てくるのと、ControllerがFatになりすぎると考えており、そこが課題になると考えている。
• テストが容易なコードは必然的に疎結合になる
• 疎結合にする場合、命名を抽象的にしておくと処理の入れ替えが容易になる（命名が具象、つまり実装の詳細に依存しないため）
• 疎結合にするとパーツが増えるので認知負荷が上がる
• 疎結合でかつ、命名が抽象化されている場合、仕様を知らない人にとっては実際の処理内容を推測しづらくなる
  • つまりこれは属人性が増えると考える
• 例外についてはErrorクラスを継承し、カスタム例外を作成して、用途に応じたハンドリングができるようにする
  • 原則として処理を止める場合にのみ用いるべきで、続行する場合には使わない
  • 例外は原則としてスローして、カスタムエラーは特定の階層でフィルタしてハンドリング、全てすり抜けてきたものはルート処理でキャッチしてハンドリングすることで、取りこぼしをゼロにする。例外の握り潰しは原則行わない
    • 基本的にすべてロギングする
  • 処理を継続するものについては例外とせず、ワーニング用の処理フローを作成し、それに則って行う（例えば入力バリデーションはワーニング）

五年くらい公私ともにTypeScriptを書いているが、なんか最近限界を感じたので吐露していく。半分くらいNode.jsに対する不満かもしれない。内容としてはただの書きなぐりの愚痴。

非同期制御多すぎ

asyncとawaitを書く面倒さや、それが漏れた時のふるまいの微妙さ。

例えば終わらないJestというのは日常的に見る光景だと思うし、実務をしていてPromiseの例外ハンドリングをしていなかったからサーバーがクラッシュしたというのも聞いたことがある。

これはチーム開発において、チームの意識や技術レベルが低いと容易に引き起こされる問題であり、その面で考えると非常に微妙な言語仕様だと思う。

なんだかんだC10K問題はコンテナ大量起動で回避する世界線になってしまっている気がするので、標準が非同期である必要性はあんまないよねみたいな感触は抱いている。C10K問題回避のために非同期にしてるんだよね？そうだよね？（正直非同期にしたいのであればユーザー側がPromiseに包めばいいと思う）

Node.jsのLTS短すぎ

GitHubやAWSですら最新版への追従は遅れがちなくらいLTSが短い。2年くらいしかない。2年ごとにフルリグレッションなんてやってられない。不可能だ。

しかも破壊的変更を平気で入れてくるし、v16は予定より早くサポートが打ち切られた。正気とは思えない。

実行環境多すぎ

Node.js + 各種トランスパイラにDenoにBunに…他にも最近なんか出ていた気がするが忘れた。なんでこんなにあるのか…。

TypeScriptの型パズルに疲れた

型パズルするために開発してるんじゃないんだよ…。

型が信用できない

実務だと意図しているのか、していないのか型を騙している光景を多々見るが、これによって型が信用できない状態になっていることが往々にしてあり、割と殺意を覚える。なにも信用できない。何のためにこの言語使ってるんだっけ…？ナウいから？

CLI作る時のハマりどころが多い

nvmのようなバージョン管理システムを使っているとグローバルインストールが通常ユーザーとスーパーユーザーで別れる問題があり、sudo実行が前提のCLIで誤った動作確認をしてしまいやすい。

他にもnpmjsで公開しているパッケージをnpm installしたものと、開発中に実行するのでは、意識してないと実行方式が変わってしまい、上手く動かないことがある。気を付ければいいのはそうだが、割とムカつく。

具体的にはshebang書いて無くて動いてなかったとかそういうやつ。基本的にjsファイルを直に叩いて動かす文化ないじゃん…。

ビルド工程が面倒くさい

ビルドに関してtscやswc、esbuildのように様々なトランスパイラが存在するほか、webpackやvite, esbuildといったバンドラの存在があり、バージョンアップで振る舞いが変わるので面倒くさい。TypeScript開発は膨大なパッケージに依存しがちで、何か不具合が起きた時に原因の特定が困難だが、根幹に破壊的変更が来ると割と精神的にきつい。

先ほどswcでビルドしたところ/dist/index.jsとして出てくるはずのものが、/dist/src/index.jsになっていた。正直こんなのはデグレードだ。ありえない。

もちろんこれはfix: pass in --strip-leading-paths to swc when buildingで対応されているのだが、正直しんどい。

てかNode.js系のパッケージって息を吸うようにデグレ起こすような気がするのは私だけだろうか。以前はNewRelicがプリフェッチしまくるバグとかあった気がするし、Jestのモックが正常に動かないケースにも遭遇したことがある。

エコシステムが壊れかかっている

TypeScriptでLintをするとなればESLintがデファクトスタンダードだと思うが、v8で導入されたFlat configがあまりにも不案内で、v9.7が出た今でさえ対応できていないPluginがゴロゴロある。むしろできてるほうが珍しい。

私も三日くらい格闘したが、いずれかのプラグインが不整合を起こすか、不整合を起こさなくなったと思ったら無視設定が機能しなかったり、今度はLint機能そのものが死んだりで手に負えないのであきらめた。v9に上げると使えなくなるプラグインもあるので、v8.57.0で旧式の設定を使うのがきっと無難だと思うが、これがいつまで続くのかという話だ。恐らく追従できないプラグインはサポートのやる気をなくすだろう。

Prettierも破壊的変更を繰り返しフォーマットをガタガタにしてくる困りものだ。こいつもv3が出て一年たった今でもVSCodeの公式拡張機能がv3に対応していない。一応ローカルにv3を入れれば最低限動くが、ネイティブで動いてほしいケースもあるので、そういう場合は運用に難儀する。過去に対応させようと手を付けてみたが、最新版はテストが落ちる状態になっていて、もはや救いがなかったので投げた。

正直ESLintとPrettierが死にかけている現状を見るに、もうなんというか駄目じゃないかという気がしてくる。

Biome.jsなんてのもあるが、あれはMarkdownのフォーマットができないとか、ESLintのプラグインをサポートしてないとかで使い勝手がいまいちよくないのだ。まぁ、MarkdownのフォーマットだけPrettier使うとかはアリか。

あとがき

とはいえ、フロントエンド開発において、ブラウザではJavaScriptしか動かないのでTypeScriptをの利用はやむを得ない側面もある。やむを得ないのだ。ただ、それ以外ではもう使う必要ないんじゃないかなと思うし、正直Node.js界隈が魔境というか地獄過ぎて、あんまり関わり合いになりたくないという思いが日に日に強くなっている今日この頃だ。

process.exit()のラッパーだったり、throwする関数だったりして、戻り値がneverを取る関数で、TypeScript上、後続処理がデッドロジックになるものを作る方法。

確認環境

やりたいこと

process.exit()を書くと次の行以降がデッドロジック扱いされるが、これをやりたい。

問題点

普通に実装して型推論に任せてもうまくいかない。

例えば次の左図の関数の戻り値型はneverだが、右図ではデッドロジックとならない。

これは次のように戻り値の型にneverを直書きしても機能しない。

解決策

次の左図のように関数そのものの型を作ると解決できる。右図を見るとデッドロジックになっていることが確認できる。

何のためにこれをやるか

別の関数から呼んだときに、呼び側の関数の戻り値型を正しくするためだ。下図はデッドロジックになっていない状態のときのものだが、処理が死なずに通り抜けてしまうのでundefinedが帰ることになっており、この関数の呼び下でundefinedだった時の処理を書く必要が出てきてしまう。

しかし、デッドロジック扱いになっていれば下図のように戻り値の型が期待した通りの内容になる。この関数がundefinedを返すことはないので、この状態を実現するために行っている。

関連記事

• TypeScriptでprocess.exit()ラッパーの後をデッドロジック扱いにする
  • オブジェクトをラップして、そのオブジェクトに型を付けることで解決している

依存ライブラリの関係でESLint v9への移行はできていないが、取り合えずFlat configに移行した。ESLintは7.9.0から8.57.0に更新している。

確認環境

Flat configになってからeslint:recommendedは@eslint/jsに移った模様。

元の設定

{
  "extends": [
    "eslint:recommended",
    "plugin:@typescript-eslint/recommended",
    "plugin:jest/style",
    "plugin:jest/recommended",
    "@lycolia/eslint-config",
    "prettier"
  ],
  "plugins": [
    "@typescript-eslint"
  ],
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "project": "./tsconfig.json"
  }
}

やったこと

ESLintなど必要ライブラリのアップデート

npm i -D eslint @eslint/js eslint-plugin-jest jest prettier typescript typescript-eslint @lycolia/eslint-config

新設定の作成

eslint.config.jsを作成

extendsの設定

今までextendsに書いていたものはrequireでモジュールを参照してmodule.exports = []の配列の中に書いた。typescript-eslintとeslint-plugin-jestの対応が何とも言えない。なお、この状態だとtypescript-eslintが正常に動かない。

const eslint = require("@eslint/js");
const eslintTypeScript = require("typescript-eslint");
const eslintJest = require("eslint-plugin-jest");
const eslintLycolia = require("@lycolia/eslint-config");

module.exports = [
  eslint.configs.recommended,
  eslintTypeScript.configs.recommended[0],
  eslintJest.configs["flat/style"],
  eslintJest.configs["flat/recommended"],
  eslintLycolia,
];

設定が競合して都合が悪かったので、この時にPrettierの設定を削除しているが、Flat config化とは特に関係ないので無視してよい。

@lycolia/eslint-configは私が作っているカスタム設定だが、これは特に何もせずとも使えた。

.eslintignore対応

extendsしたオブジェクトの次にオブジェクトを記述し、そこにignores: []を追加し、その中に同じことを書いた。恐らく根っこの配列はESLintの設定オブジェクトの羅列で、最終的に後勝ちでマージされる様に出来ているのだろう。

const eslint = require("@eslint/js");
const eslintTypeScript = require("typescript-eslint");
const eslintJest = require("eslint-plugin-jest");
const eslintLycolia = require("@lycolia/eslint-config");

module.exports = [
  eslint.configs.recommended,
  eslintTypeScript.configs.recommended[0],
  eslintJest.configs["flat/style"],
  eslintJest.configs["flat/recommended"],
  eslintLycolia,
  {
    ignores: [
      'dist/*',
      'coverage/*',
      '**/*.d.ts',
      '/src/public/',
      '/src/type',
      '*.config.js'
    ],
  }
];

TypeScript対応

eslintTypeScript.config()でESLintの設定配列全体を引数としてラップし、languageOptionsの中身を移植した。旧設定にあったpluginsは公式のドキュメントを見る限り不要になっている模様。

const eslint = require('@eslint/js');
const eslintTypeScript = require('typescript-eslint');
const eslintJest = require('eslint-plugin-jest');
const eslintLycolia = require('@lycolia/eslint-config');

module.exports = eslintTypeScript.config(
  eslint.configs.recommended,
  ...eslintTypeScript.configs.recommended,
  eslintJest.configs['flat/style'],
  eslintJest.configs['flat/recommended'],
  eslintLycolia,
  {
    ignores: [
      'dist/*',
      'coverage/*',
      '**/*.d.ts',
      '/src/public/',
      '/src/type',
      '*.config.js'
    ],
    languageOptions: {
      parser: eslintTypeScript.parser,
      parserOptions: {
        project: './tsconfig.json'
      }
    }
  }
);

旧設定の削除

.eslintrcと.eslintignoreを消した。

ESLint v9対応

typescript-eslintが対応していないため、現時点ではできなかった。

変更差分まとめ

-{
-  "extends": [
-    "eslint:recommended",
-    "plugin:@typescript-eslint/recommended",
-    "plugin:jest/style",
-    "plugin:jest/recommended",
-    "@lycolia/eslint-config",
-    "prettier"
-  ],
-  "plugins": [
-    "@typescript-eslint"
-  ],
-  "parser": "@typescript-eslint/parser",
-  "parserOptions": {
-    "project": "./tsconfig.json"
+const eslint = require('@eslint/js');
+const eslintTypeScript = require('typescript-eslint');
+const eslintJest = require('eslint-plugin-jest');
+const eslintLycolia = require('@lycolia/eslint-config');
+
+module.exports = eslintTypeScript.config(
+  eslint.configs.recommended,
+  ...eslintTypeScript.configs.recommended,
+  eslintJest.configs['flat/style'],
+  eslintJest.configs['flat/recommended'],
+  eslintLycolia,
+  {
+    ignores: [
+      'dist/*',
+      'coverage/*',
+      '**/*.d.ts',
+      '/src/public/',
+      '/src/type',
+      '*.config.js'
+    ],
+    rules: {
+      semi: ['error', 'always']
+    },
+    languageOptions: {
+      parser: eslintTypeScript.parser,
+      parserOptions: {
+        project: './tsconfig.json'
+      }
+    }
   }
-}
+);