-
Notifications
You must be signed in to change notification settings - Fork 346
feat: electron builderの設定をtypescriptで書くようにする #2708
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: electron builderの設定をtypescriptで書くようにする #2708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちなみにこれって、起こり得る致命的な問題とかありますかね?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A migration of the Electron Builder setup and related build scripts from CommonJS to TypeScript/ESM, with a dynamic loader for the TS config.
- Updated the
electron:buildscript inpackage.jsonto point at a new CJS loader and addedjitifor TS imports. - Converted split/archive and post-build hooks (
splitNsisArchive,afterAllArtifactBuild,afterNsisWebArtifactBuild) to TypeScript with ESM imports and type annotations. - Introduced
electronBuilderConfigLoader.cjsto load a TS-basedelectronBuilderConfig.ts, removed legacy CJS config files.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Switched build command to custom CJS loader, added jiti dependency |
| build/splitNsisArchive.ts | Migrated to TS/ESM, added type annotations, improved error throws |
| build/electronBuilderConfigLoader.cjs | New loader module to import TS config via jiti |
| build/electronBuilderConfig.ts | TS-based Electron Builder config with imports, replaced __dirname |
| build/afterNsisWebArtifactBuild.ts | New TS hook wrapping splitNsisArchive |
| build/afterNsisWebArtifactBuild.cjs | Removed legacy CJS wrapper |
| build/afterAllArtifactBuild.ts | Migrated main post-build hook to TS, returns array |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
build/splitNsisArchive.ts:22
- [nitpick] The constant name
ErrorMessageis in PascalCase, while the earlier error uses lowerCamelCase (errorMessage). Rename this toerrorMessagefor consistency.
const ErrorMessage = "Project version is undefined.";
build/afterAllArtifactBuild.ts:18
- Returning an empty array from the
afterAllArtifactBuildhook overrides the default artifact list behavior. Confirm whether you intend to return no additional artifacts or should simply return void to preserve existing artifacts.
return [];
build/electronBuilderConfig.ts:7
- import.meta.dirname is not a standard property in ES modules. To get the current file directory, use something like:
import { fileURLToPath } from 'url';
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const rootDir = path.join(__dirname, '..');const rootDir = path.join(import.meta.dirname, "..");
|
思いつく限りではないですね。electron-builderの視点ではコンフィグを返す(非同期な)関数またはコンフィグそのものを受け取っていれば問題ない、そしてこれもそのコンフィグを返す関数というのは変わっていないので。 |
|
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ: |
|
なるほどです! jiti採用はわりと良さそうなんですが、念の為メンテナとして、ライブラリの選定基準をどれくらい満たしてるのか気にしておいても良いかもと思いました! |
|
📝 以前jitiについて話した記憶があったのでメモ |
なので問題なさそう? |
Hiroshiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なので問題なさそう?
なるほどです、良さそう!!!
ライブラリによってはテストコードの充実度を見ておくのも良さそう。(electron-storeの経験 😇 )
LGTM!!
1つzodの提案していますが、もしよければくらいの気持ちです!
| // else: nop | ||
| } | ||
| }; | ||
| return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝
これ何が入るんだろうと思ったら // you can return additional files to publish とのこと。
分かりづらい!
|
特に問題なさそうなのでマージします。 |
内容
タイトル通りです。
関連 Issue
(なし)
スクリーンショット・動画など
(なし)
その他
(なし)