feat: support TypeScript migration files based on tsconfig presence - #1639
beterrabaA wants to merge 10 commits into
Conversation
|
I'm adding documentation about this.Don't merge yet. |
|
It's done. |
|
Problems and questions:
|
Please use drafts 😉 |
|
@beterrabaA I summon the cat back 🙌 It would be a good time right now to rebase this PR and move on 🙂 |
|
🫡 |
|
Tomorrow, I will deliver it. Sorry for the wait. 
|
…esence on `migration.ts` file
f3f64d6 to
c8b73dd
Compare
…le generation logic
… in tsconfig parsing
|
Sorry for not adding monorepo support.I never worked with it before. Now it's checking if there's a tsconfig file. If there's, will verify the allowJs property. If there's not, it will search for 
|
Shinigami92
left a comment
Shinigami92
left a comment
There was a problem hiding this comment.
🤖 Claude Opus 5 Review:
Verdict: request changes. The goal is good and the precedence design is right, but there's a release-blocking crash and the core heuristic is inverted. Lint, tsc, and all 749 unit tests pass — the problems are all in behavior I had to probe for.
What's right
resolveSuffix keeps the order language || getLastSuffix() || <detected> (src/migration.ts:184), so an explicit -j wins, then the suffix of the existing migrations, and detection only kicks in for an empty migrations dir. That's the correct place to put this, and it means existing projects are untouched.
Blockers
1. Importing the package crashes on a valid tsconfig.json
src/migration.ts:74 calls shouldGenerateTypescript() at module top level, and src/index.ts:1 re-exports Migration from that module — so this runs on every import 'node-pg-migrate'. The comment-stripping regex in src/utils/shouldGenerateTs.ts:17 hands the result to JSON.parse, which rejects trailing commas that tsc accepts happily. Verified against src/index.ts with cwd set to a project whose tsconfig is:
{ "compilerOptions": { "strict": true, "allowJs": true, }, }IMPORT CRASHED at module-evaluation time:
SyntaxError: Expected double-quoted property name in JSON at position 68
This takes out up, down, redo and the programmatic API, not just create. Trailing commas in tsconfig are very common. Needs a real JSONC parse (or at minimum a try/catch that degrades to js) — and it must not run at import time.
2. Module-level side effect with an unbounded filesystem scan
src/migration.ts:74 and :176 both execute at import. globSync('**/*.ts') walks the whole tree from cwd on every import, including for commands that never read the value. Measured in this repo: 13.8ms, 270 matches — and it grows with the project. It also freezes cwd at import time, so a later chdir is ignored. Move the call inside resolveSuffix; it's already async and only needed on the create path.
3. allowJs is the wrong signal, and it's used inverted
allowJs: true means "tsc will also accept .js files" — routinely enabled in TypeScript projects that are mid-migration or that consume untyped JS. Treating it as "this is a JS project" gets the common case backwards, and the existsTsConfig && allowsJsInTsConfig branch discards hasTypeScriptFiles entirely:
| project | actual | expected |
|---|---|---|
tsconfig.json + src/index.ts, allowJs: true |
js |
ts — it is a TS project |
allowJs: true inherited via extends |
ts |
js by the PR's own rule |
plain JS project with one env.d.ts |
ts |
js |
Suggestion: drop the allowJs inspection and the whole JSONC-parsing problem with it. existsSync('tsconfig.json') ? 'ts' : 'js' matches what the PR description promises, is what users will predict, and anyone who disagrees already has -j js plus the existing-migration-suffix signal ahead of it.
Should fix
4. extends is not resolved
shouldGenerateTs.ts:22 reads compilerOptions.allowJs off the parsed file only. Verified: a base config with allowJs: true pulled in via extends returns true. Real configs put compilerOptions in an @tsconfig/node22-style base. Resolving extends chains correctly is a genuine amount of work, which is another argument for #3.
5. **/*.ts over-matches
Beyond the .d.ts case in the table above, the ignore list is only node_modules and dist; build, out, .next, coverage, .turbo all still count, and .gitignore isn't consulted.
6. Import style breaks repo convention
shouldGenerateTs.ts:1 uses from 'fs' — every other file in src/ uses the node: prefix. And :2 imports glob/raw while src/migration.ts:1 imports glob; both are valid subpaths but they resolve to the unminified and minified builds respectively, so the dist bundle ends up carrying two copies of glob. Use 'glob'.
7. Take cwd as a parameter
shouldGenerateTypescript(cwd = process.cwd()) would replace the hardcoded './tsconfig.json' and let the test drop process.chdir (test/utils/fileExtension.spec.ts:16). That chdir works today only because vitest 4 defaults to the forked pool with isolation — it becomes a hard failure under pool: 'threads', where process.chdir doesn't exist.
Minor
- 8.
const lastSuff = ...(src/migration.ts:176) is wedged between two function declarations, and the name is cryptic. Inline it intoresolveSuffixas part of fixing #2. - 9. Three names for one thing: file
shouldGenerateTs.ts, exportshouldGenerateTypescript, testfileExtension.spec.ts. The repo names each util file after its export. - 10. Not re-exported from
src/utils/index.ts, unlike every other util, somigration.ts:19has to deep-import it. - 11. Docs still document the old default:
docs/src/cli.md:136(migration-file-language→js) and:179. Since this changes a default, it also belongs indocs/src/upgrading.md.
Suggested shape
// src/utils/shouldGenerateTs.ts
import { existsSync } from 'node:fs';
import { join } from 'node:path';
/** Detect whether a new migration should default to TypeScript. */
export function shouldGenerateTypescript(cwd: string = process.cwd()): boolean {
return existsSync(join(cwd, 'tsconfig.json'));
}// src/migration.ts — no module-level work
async function resolveSuffix(
directory: string,
options: CreateOptionsDefault
): Promise<string> {
const { language, ignorePattern } = options;
return (
language ||
(await getLastSuffix(directory, ignorePattern)) ||
(shouldGenerateTypescript() ? 'ts' : 'js')
);
}That keeps the PR's headline behavior, removes the crash, the import-time scan, the extends gap, and the .d.ts false positive. The test file then reduces to a cwd-argument table and loses its chdir fragility — though it's worth keeping a case asserting an existing .js migration still wins over a present tsconfig, since that's the compatibility guarantee for current users.
Verification notes
How the claims above were checked, for anyone re-running them:
- Blockers 3–5 and the table: the util's body copied verbatim into a standalone script, run against generated fixture projects (
tsconfigwith trailing comma,allowJsviaextends, JS project plus one.d.ts, TS project withallowJs: true). - Blocker 1:
jiti.import('src/index.ts')with cwd set to a fixture project holding a trailing-comma tsconfig. - Blocker 2:
globSync('**/*.ts', { ignore: ['**/node_modules/**', '**/dist/**'] })timed withprocess.hrtime.bigint()at the repo root. - Baseline:
pnpm lint,pnpm ts-check,pnpm vitest run --project unit— all green on the PR branch.
This branch has not been deployed
2 participants
node-pg-migrate createnow generates by default.jsor.tsdepending on the presence of thetsconfig.jsonfile