Skip to content
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

feat: インポート一覧をチェックするように #2513

Merged
merged 10 commits into from
Jan 31, 2025
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

"license:generate": "tsx tools/generateLicenses.mts",
"license:merge": "tsx tools/mergeLicenses.mts",
"check-suspicious-imports": "tsx tools/checkSuspiciousImports.mts",

"test:unit": "vitest --run",
"test-watch:unit": "vitest --watch",
Expand Down Expand Up @@ -114,6 +115,8 @@
"@vue/eslint-config-prettier": "9.0.0",
"@vue/eslint-config-typescript": "13.0.0",
"@vue/test-utils": "2.4.5",
"acorn": "8.14.0",
"acorn-walk": "8.3.4",
"chromatic": "11.5.4",
"cross-env": "7.0.3",
"dotenv": "16.0.0",
Expand Down
14 changes: 14 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

209 changes: 209 additions & 0 deletions tools/checkSuspiciousImports.mts
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
/**
* 与えられたjsファイルのimport/requireを解析し、以下の条件に当てはまらないものがあれば警告を出力する:
* - Node.js 標準である
* - electron
* - try-catch内での明示的に許可されたパッケージ
*
* また、try-catch内での明示的に許可されたパッケージが使われていない場合も警告を出力する。
*
* pnpm run check-suspicious-imports ./path/to/file.js のように単体でも実行可能。
*
* 文脈:
* electron-builderはpnpmを一緒に使うとnode_modulesが壊れる問題があるが、
* それはビルド後(dist/{main.js,preload.js})でパッケージがimport/requireされなければ問題ないため、
* このスクリプトを使って問題がないかチェックする。
* ref: https://github.com/VOICEVOX/voicevox/issues/2508
sevenc-nanashi marked this conversation as resolved.
Show resolved Hide resolved
*/

import { builtinModules } from "module";
import { styleText } from "util";
sevenc-nanashi marked this conversation as resolved.
Show resolved Hide resolved
import fs from "fs/promises";
import yargs from "yargs";
import { parse } from "acorn";
import { ancestor as visitWithAncestor } from "acorn-walk";
import { hideBin } from "yargs/helpers";

const defaultAllowedModules = [
...builtinModules.map((m) => `node:${m}`),
"electron",
];

type Import = {
path: string;
isInsideTryCatch: boolean;
};
export type CheckSuspiciousImportsOptions = {
allowedModules?: string[];
allowedInTryCatchModules?: string[];
list?: boolean;
};
export function checkSuspiciousImports(
Copy link
Member

Choose a reason for hiding this comment

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

(判断コメント)

このファイルのコードは相当文脈が深く、何やってるか雰囲気はわかるけどぶっちゃけメンテは大変そうな気がしています。
特にacornライブラリを使う周りが専門知識。(どうしようもない)

なのでこのコードのメンテは @sevenc-nanashi さんに頼らせていただけると頼もしいです!!

file: string,
jsContent: string,
options: CheckSuspiciousImportsOptions = {},
): void {
console.log(`Checking suspicious imports in ${file}...`);
const ast = parse(jsContent, {
ecmaVersion: "latest",
sourceType: "module",
});

const allImports: Import[] = [];

const allowedModules = [
...defaultAllowedModules,
...(options.allowedModules ?? []),
];
const allowedInTryCatchModules = options.allowedInTryCatchModules ?? [];

visitWithAncestor(ast, {
ImportDeclaration(node) {
const importPath = node.source.value;
if (typeof importPath === "string") {
allImports.push({
path: importPath,
isInsideTryCatch: false,
});
}
},
CallExpression(node, _, ancestors) {
const isInsideTryCatch = ancestors.some(
(ancestor) => ancestor.type === "TryStatement",
);
if (node.callee.type === "Identifier" && node.callee.name === "require") {
const importPath =
node.arguments[0].type === "Literal" && node.arguments[0].value;
if (typeof importPath === "string") {
allImports.push({
path: importPath,
isInsideTryCatch,
});
}
}
},
});

const normalizedImports: Import[] = [];
for (const importInfo of allImports) {
let path = importInfo.path;
if (builtinModules.includes(path) && !path.startsWith("node:")) {
path = `node:${path}`;
}
const existingImport = normalizedImports.find((i) => i.path === path);
if (existingImport) {
existingImport.isInsideTryCatch &&= importInfo.isInsideTryCatch;
} else {
normalizedImports.push({
...importInfo,
path,
});
}
}

const allowedImports = normalizedImports.filter((i) =>
allowedModules.includes(i.path),
);
const allowedInTryCatchImports = normalizedImports.filter(
(i) =>
!allowedModules.includes(i.path) &&
allowedInTryCatchModules.includes(i.path),
);
const suspiciousImports = normalizedImports.filter(
(i) =>
!allowedModules.includes(i.path) &&
!allowedInTryCatchModules.includes(i.path),
);

for (const [color, title, imports] of [
["green", "Allowed", allowedImports],
["yellow", "Allowed in try-catch", allowedInTryCatchImports],
["red", "Suspicious", suspiciousImports],
] as const) {
if (options.list) {
console.log(styleText(color, ` ${title}:`));
for (const i of imports) {
console.log(styleText(color, ` ${i.path}`));
}
if (imports.length === 0) {
console.log(styleText(color, ` (none)`));
}
} else {
console.log(styleText(color, ` ${title}: ${imports.length}`));
}
}

if (suspiciousImports.length > 0) {
throw new Error(
`Suspicious imports found: ${suspiciousImports
.map((i) => i.path)
.join(", ")}`,
);
}

const unusedAllowedInTryCatchModules = allowedInTryCatchModules.filter(
(m) => !allowedInTryCatchImports.some((i) => i.path === m),
);
if (options.list) {
console.log(styleText("yellow", ` Unused allowed in try-catch modules:`));
for (const m of unusedAllowedInTryCatchModules) {
console.log(styleText("yellow", ` ${m}`));
}
if (unusedAllowedInTryCatchModules.length === 0) {
console.log(styleText("yellow", ` (none)`));
}
} else {
console.log(
styleText(
"yellow",
` Unused allowed in try-catch modules: ${unusedAllowedInTryCatchModules.length}`,
),
);
}
if (unusedAllowedInTryCatchModules.length > 0) {
throw new Error(
`Some allowed in try-catch modules are not used in try-catch block: ${unusedAllowedInTryCatchModules.join(
", ",
)}`,
);
}
}

if (import.meta.filename === process.argv[1]) {
const parser = yargs(hideBin(process.argv))
.option("allowed-modules", {
type: "string",
array: true,
description: "Allowed modules",
alias: "a",
})
.option("allowed-in-try-catch-modules", {
type: "string",
array: true,
description: "Allowed in try-catch modules",
alias: "t",
})
.positional("files", {
type: "string",
description: "Files to check",
array: true,
})
.strictCommands()
.demandCommand(1);
const args = await parser.parse();

for (const rawFile of args._) {
const file = rawFile.toString();
fs.readFile(file, "utf-8")
.then((content) => {
checkSuspiciousImports(file, content, {
allowedModules: args.allowedModules,
allowedInTryCatchModules: args.allowedInTryCatchModules,
list: true,
});
})
.catch((e) => {
console.error(e);
process.exit(1);
});
}
}
37 changes: 35 additions & 2 deletions vite.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { BuildOptions, defineConfig, loadEnv, Plugin } from "vite";
import { quasar } from "@quasar/vite-plugin";
import { z } from "zod";

import {
checkSuspiciousImports,
CheckSuspiciousImportsOptions,
} from "./tools/checkSuspiciousImports.mjs";

const isElectron = process.env.VITE_TARGET === "electron";
const isBrowser = process.env.VITE_TARGET === "browser";

Expand Down Expand Up @@ -97,7 +102,15 @@ export default defineConfig((options) => {
}
},
vite: {
plugins: [tsconfigPaths({ root: import.meta.dirname })],
plugins: [
tsconfigPaths({ root: import.meta.dirname }),
checkSuspiciousImportsPlugin({
allowedInTryCatchModules: [
// systeminformationのoptionalな依存。try-catch内なので許可。
"osx-temperature-sensor",
],
}),
Hiroshiba marked this conversation as resolved.
Show resolved Hide resolved
],
build: {
outDir: path.resolve(import.meta.dirname, "dist"),
sourcemap,
Expand All @@ -113,7 +126,10 @@ export default defineConfig((options) => {
}
},
vite: {
plugins: [tsconfigPaths({ root: import.meta.dirname })],
plugins: [
tsconfigPaths({ root: import.meta.dirname }),
checkSuspiciousImportsPlugin({}),
],
build: {
outDir: path.resolve(import.meta.dirname, "dist"),
sourcemap,
Expand Down Expand Up @@ -156,3 +172,20 @@ const injectBrowserPreloadPlugin = (): Plugin => {
},
};
};

const checkSuspiciousImportsPlugin = (
options: CheckSuspiciousImportsOptions,
): Plugin => {
return {
name: "check-suspicious-imports",
enforce: "post",
apply: "build",
writeBundle(_options, bundle) {
for (const [file, chunk] of Object.entries(bundle)) {
if (chunk.type === "chunk") {
checkSuspiciousImports(file, chunk.code, options);
}
}
},
};
};
Loading