Skip to content

feat(template)!: unify JS/TS templates#4229

Open
erickzhao wants to merge 23 commits into
nextfrom
template-unification
Open

feat(template)!: unify JS/TS templates#4229
erickzhao wants to merge 23 commits into
nextfrom
template-unification

Conversation

@erickzhao
Copy link
Copy Markdown
Member

@erickzhao erickzhao commented Apr 20, 2026

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 --typescript as 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's stripTypeScriptTypes function.

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.

@github-actions github-actions Bot added the next label Apr 20, 2026
@socket-security
Copy link
Copy Markdown

socket-security Bot commented May 21, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​electron-forge/​plugin-webpack@​0.0.0-use.local100100100100100

View full report

@erickzhao erickzhao marked this pull request as ready for review May 22, 2026 00:14
@erickzhao erickzhao requested a review from a team as a code owner May 22, 2026 00:14
Comment thread packages/template/base/package.json Outdated
Comment thread package.json Outdated
@erickzhao erickzhao requested a review from erikian May 23, 2026 00:25

const d = debug('electron-forge:template:base');
const tmplDir = path.resolve(import.meta.dirname, '../tmpl');
const oxfmtConfig = readJsonSync(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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' });
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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[] {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread packages/template/webpack/src/WebpackTemplate.ts
Comment thread vitest.config.mts Outdated
@erickzhao
Copy link
Copy Markdown
Member Author

@claude review

Comment on lines +47 to +52
const tsMatch = template.match(/^(.+)-typescript$/);
if (tsMatch) {
throw new Error(
`The "${template}" template no longer exists. Use "--template ${tsMatch[1]} --typescript" instead.`,
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 nit: the new tsMatch regex /^(.+)-typescript$/ runs on the raw template string regardless of what was passed in, so it misfires in two narrow ways: (1) absolute paths — --template /Users/me/projects/foo-typescript produces the nonsense suggestion Use --template /Users/me/projects/foo --typescript, and (2) any third-party template whose package name ends in -typescript (e.g. electron-forge-template-foo-typescript) gets the same misleading advice, even though third-party templates don't honor the new InitTemplateOptions.typescript flag. Since the deprecation only applies to the bundled vite and webpack templates, gating on a known-name allowlist — or at minimum on !isAbsolutePath (already computed on line 24) — would tighten this up.

Extended reasoning...

What the bug is

At find-template.ts:47-52, after the standard template-resolution loop fails to locate a template, the code tries to be helpful:

const tsMatch = template.match(/^(.+)-typescript$/);
if (tsMatch) {
  throw new Error(
    `The "${template}" template no longer exists. Use "--template ${tsMatch[1]} --typescript" instead.`,
  );
}

The intent (per the PR — unifying vite/webpack with their -typescript siblings into a single --typescript flag) is to redirect users who type the old short-form names vite-typescript / webpack-typescript. But the regex is unanchored to those two specific names — it matches any failed template string ending in -typescript.

Why existing code doesn't prevent it

isAbsolutePath is computed on line 24 and is used to switch between file-URL resolution and the bare/prefixed package-name lookup. But it is not consulted by the tsMatch block, so the heuristic fires for absolute paths as well. Likewise there is no allowlist of bundled forge template names — the regex matches foo-typescript, my-cool-typescript, electron-forge-template-foo-typescript, etc.

Step-by-step proof — absolute path

  1. User runs: create-electron-app myapp --template /Users/me/projects/foo-typescript
  2. template = '/Users/me/projects/foo-typescript'
  3. isAbsolutePath = true, so the resolve list is just [templatePath] (the file URL of that path).
  4. import.meta.resolve fails (no module at that location).
  5. foundTemplate is null, so we hit the tsMatch block.
  6. The regex matches with tsMatch[1] = '/Users/me/projects/foo'.
  7. Error thrown: The "/Users/me/projects/foo-typescript" template no longer exists. Use "--template /Users/me/projects/foo --typescript" instead.

That advice is wrong on every axis: the truncated path is a different (probably also non-existent) location, and --typescript only has meaning for the bundled vite/webpack templates anyway (it is consumed by InitTemplateOptions.typescript inside the bundled templates' initializeTemplate).

Step-by-step proof — custom template name

  1. User runs: create-electron-app myapp --template electron-forge-template-foo-typescript (a third-party package that isn't installed).
  2. All three resolve attempts fail.
  3. tsMatch[1] = 'electron-forge-template-foo'.
  4. Error thrown: Use "--template electron-forge-template-foo --typescript" instead.

Again misleading: electron-forge-template-foo may not exist either, and even if it does, it almost certainly won't read options.typescript from initializeTemplate — that contract was added by this PR for the two bundled templates.

Impact

Low — this only fires on the failure path (the user's original command was already broken), and following the misleading suggestion will fail loudly rather than silently. It's a UX papercut around error guidance, not a correctness bug.

Suggested fix

Gate the heuristic on the actually-affected names, e.g.:

if (!isAbsolutePath && (template === 'vite-typescript' || template === 'webpack-typescript')) {
  const bundler = template.replace(/-typescript$/, '');
  throw new Error(
    `The "${template}" template no longer exists. Use "--template ${bundler} --typescript" instead.`,
  );
}

Either narrowing to the known names or simply gating on !isAbsolutePath fixes both reported cases.


console.log(
'👋 This message is being logged by "renderer.js", included via webpack',
'👋 This message is being logged by "renderer.ts", included via webpack',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Nit: in , the console.log references the filename as "renderer.ts" (this PR changed the webpack one from "renderer.js" to "renderer.ts"). For the JS variant, stripTypeScriptTypes only removes TS syntax — it does not rewrite string literals — so the generated renderer.js in a JS scaffold will print '…logged by "renderer.ts"…'. Cosmetic but a small regression vs the pre-PR JS templates, which correctly said renderer.js. Easiest fix: use a filename-neutral message (e.g. '…logged by the renderer process…'), or patch the string in the JS branch of stripAndRename similar to how forge.config paths are rewritten.

Extended reasoning...

What the bug is

The Vite renderer template (packages/template/vite/tmpl/renderer.ts:18) and the Webpack renderer template (packages/template/webpack/tmpl/renderer.ts:18) both contain a hard-coded filename in their hello-world console.log:

console.log(
  '👋 This message is being logged by "renderer.ts", included via Vite',
);

For the webpack template, the PR diff explicitly changed this line from '…logged by "renderer.js"…' to '…logged by "renderer.ts"…' (see the diff at packages/template/webpack/tmpl/renderer.ts). The Vite one already had "renderer.ts".

Why the JS variant ends up with the wrong message

For the JS scaffold (create-electron-app myapp --template vite or --template webpack with no --typescript), ViteTemplate.initializeTemplate / WebpackTemplate.initializeTemplate iterate over ['main', 'renderer', 'preload'] and call this.stripAndRename(\${name}.ts`, `${name}.js`). stripAndRename(inBaseTemplate.ts`) does:

const stripped = stripTypeScriptTypes(source, { mode: 'strip' });

stripTypeScriptTypes from node:module only removes TypeScript-specific syntax (type annotations, interfaces, declare statements, etc.). It does not rewrite string literals. So the body of the console.log is passed through verbatim into src/renderer.js.

Step-by-step proof

  1. User runs create-electron-app myapp --template webpack (no --typescript).
  2. init calls WebpackTemplate.initializeTemplate(dir, { typescript: false }).
  3. The task loops over ['main', 'renderer', 'preload']:
    • copyTemplateFile(srcDir, 'renderer.ts')src/renderer.ts now contains the literal string "renderer.ts" inside console.log.
    • stripAndRename('src/renderer.ts', 'src/renderer.js') strips TS types (none here) and writes src/renderer.js with the same string literal.
  4. The user runs the app and sees in the DevTools console:
    👋 This message is being logged by "renderer.ts", included via webpack
    
    …but their actual file on disk is src/renderer.js. The pre-PR JS template correctly said "renderer.js".

Why existing code doesn't catch it

  • There's no rewrite of the filename string in either ViteTemplate.ts or WebpackTemplate.ts (unlike forge.config, where paths are explicitly rewritten via updateFileByLine).
  • The unit specs only assert that the main file matches MAIN_WINDOW_*_ENTRY and (separately) the absence of type annotations — they never read the renderer file's console.log content.

Impact

Purely cosmetic — the scaffolded JS app still runs, lints, and packages fine. But the first log message a new user sees in DevTools refers to a file that doesn't exist in their project (their file is renderer.js, the message says renderer.ts). It's a small but real regression vs the pre-PR hand-maintained JS templates.

Suggested fix

Either:

  1. Make the message bundler-/filename-neutral in the .ts template, e.g.
    console.log('👋 This message is being logged by the renderer process, included via Vite');
    then both variants are correct.
  2. Or, in the JS branch of initializeTemplate, do a small post-strip rewrite of the literal — analogous to the src/main.ts → src/main.js patching already done for forge.config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants