feat(template)!: unify JS/TS templates#4229
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Co-authored-by: Erik Moura <erikian@electronjs.org>
… template-unification
|
|
||
| const d = debug('electron-forge:template:base'); | ||
| const tmplDir = path.resolve(import.meta.dirname, '../tmpl'); | ||
| const oxfmtConfig = readJsonSync( |
There was a problem hiding this comment.
Blocking: this reads the formatter config from the monorepo root (dist/ → up 4) at import time. That resolves in the workspace, but in a published install import.meta.dirname is node_modules/@electron-forge/template-base/dist, so ../../../../.oxfmtrc.json points at the directory above node_modules — and the file isn't shipped with the package (files: ["dist","src","tmpl"]; it lives at the repo root, outside the package). readJsonSync on a missing file throws, so simply importing this module crashes, which breaks create-electron-app for end users.
This won't surface in CI: the verdaccio specs load init from the workspace source, where this relative path happens to resolve. The published layout is never tested.
The previous approach (copy(tmplDir/.oxfmtrc.json, ...)) was publish-safe because tmpl/ is in files. Suggest shipping the config inside the package (keep tmpl/.oxfmtrc.json, or co-locate it in template-base and resolve relative to dist/tmpl, never the repo root), and making the load lazy + tolerant of a missing file so it can't take down module import.
| path.join(tmplDir, '.oxfmtrc.json'), | ||
| path.resolve(directory, '.oxfmtrc.json'), | ||
| ); | ||
| const { ignorePatterns: _, ...projectConfig } = oxfmtConfig; |
There was a problem hiding this comment.
Once writeLintConfig writes from oxfmtConfig, the shipped packages/template/base/tmpl/.oxfmtrc.json is no longer referenced. Either remove it, or wire it back up as the publish-safe source for the config (see the comment on the oxfmtConfig load above).
|
|
||
| async stripAndRename(srcPath: string, destPath: string): Promise<void> { | ||
| const source = await fs.readFile(srcPath, 'utf8'); | ||
| const stripped = stripTypeScriptTypes(source, { mode: 'transform' }); |
There was a problem hiding this comment.
Blocking: mode: 'transform' strips every comment. Verified on the actual templates (Node 22.22) — vite/tmpl/renderer.ts becomes just:
import './index.css';
console.log('👋 This message is being logged by "renderer.ts", included via Vite');The hand-written JS starters have always been comment-rich (Node-integration/security warnings, links to the Electron docs, "put your config here" hints). With transform mode the generated JS apps ship with none of that — a real downgrade. The tests only assert the absence of type syntax, so they stay green either way.
mode: 'strip' (the default) keeps comments and still removes declare const / type annotations; it just leaves whitespace where types were, which the existing oxfmt pass already cleans up. The templates don't use enums/namespaces/parameter-properties, so strip's stricter rules aren't a problem here. Recommend switching to strip and confirming oxfmt collapses the residual blank lines.
(Minor, same line: stripTypeScriptTypes also emits ExperimentalWarning to stderr, which users will see during scaffolding — worth suppressing around the call.)
| initOpts.copyCIFiles = Boolean(options.copyCiFiles); | ||
| initOpts.force = Boolean(options.force); | ||
| initOpts.skipGit = Boolean(options.skipGit); | ||
| initOpts.typescript = Boolean(options.typescript); |
There was a problem hiding this comment.
initOpts.typescript is set from the --typescript flag here, but further down it's unconditionally overwritten by the "Select a programming language" prompt whenever bundler !== 'base'. So in interactive mode create-electron-app myapp --typescript still prompts and the flag is effectively ignored. Consider using the flag as the prompt's default, or skipping the prompt when it was passed.
| class ViteTemplate extends BaseTemplate { | ||
| public templateDir = path.resolve(import.meta.dirname, '..', 'tmpl'); | ||
|
|
||
| public override get devDependencies(): string[] { |
There was a problem hiding this comment.
The devDependencies getter reads this._typescript, which is mutated in initializeTemplate(). These templates are exported as singletons (export default new ViteTemplate()), so this is shared mutable state — and the same pattern is in WebpackTemplate.ts. It works today only because init()'s Listr runs strictly sequentially (initialize → then install deps). It's fragile though: a concurrent init in one process, or any caller reading devDependencies before initializeTemplate, gets the wrong list. Threading typescript through explicitly (return the dep set from the init result, or a method that takes the flag) would be more robust than instance state on a singleton.
| path.resolve(directory, 'forge.config.mjs'), | ||
| (line) => { | ||
| // Remove webpack config imports (JS uses string paths instead) | ||
| if (line.includes("from './webpack.")) return null; |
There was a problem hiding this comment.
The JS forge.config is produced by string/regex rewriting of the stripped .mts (includes('mainConfig,'), regex on config: rendererConfig,, the .ts→.js replaces). Correct for the current forge.config.mts, but tightly coupled to its exact formatting — a reorder/rename there will silently break the JS output. The spec assertions on the resulting strings mitigate this; just noting the coupling.
Having separate packages for TypeScript-based templates is a burden. You need to duplicate any bundler-related template work, which leaves our templates open to drift.
This PR attempts to reconcile the two templates variants together, and to have
--typescriptas an instantiation option.The crux to this approach is that a lot of TypeScript files can be stripped by default in Node 22.13+ with
node:module'sstripTypeScriptTypesfunction.For app code: this works great.
For bundler-specific config: This works great for Vite, since it supports TS out of the box without additional configuration. For webpack, we still need to configure additional loaders, so the bundler configurations are kept separate for JS/TS.