-
Notifications
You must be signed in to change notification settings - Fork 253
👷(CLDSRV-860) Monitor async await migration #6088
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
base: development/9.3
Are you sure you want to change the base?
Changes from all commits
d3e6d78
155ad19
869e8b2
c4d81e1
8e5ad67
b15c6e8
68ce4e6
8612e12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| /** | ||
| * Check that all new/modified functions in the current git diff use async/await. | ||
| * Fails with exit code 1 if any additions introduce callback-style functions. | ||
| * | ||
| * Usage: node scripts/check-diff-async.mjs | ||
| * In CI: runs against the current PR diff (files changed vs base branch) | ||
| */ | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { Project, SyntaxKind } from 'ts-morph'; | ||
|
|
||
| const CALLBACK_PARAM_PATTERN = /^(cb|callback|next|done)$/i; | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| function getChangedJsFiles() { | ||
| const base = process.env.GITHUB_BASE_REF | ||
| ? `origin/${process.env.GITHUB_BASE_REF}` | ||
| : 'HEAD'; | ||
| const output = execFileSync('git', [ | ||
| 'diff', | ||
| '--name-only', | ||
| '--diff-filter=ACMR', | ||
| base, | ||
| '--', | ||
| '*.js', | ||
| ], { encoding: 'utf8' }).trim(); | ||
|
|
||
| return output ? output.split('\n').filter(f => f.endsWith('.js')) : []; | ||
| } | ||
|
|
||
| /** | ||
| * Get added line numbers for a file in the current diff. | ||
| */ | ||
| function getAddedLineNumbers(filePath) { | ||
| const base = process.env.GITHUB_BASE_REF | ||
| ? `origin/${process.env.GITHUB_BASE_REF}` | ||
| : 'HEAD'; | ||
| const diff = execFileSync('git', ['diff', base, '--', filePath], { encoding: 'utf8' }); | ||
| const addedLines = new Set(); | ||
| let currentLine = 0; | ||
|
|
||
| for (const line of diff.split('\n')) { | ||
| const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); | ||
|
|
||
| if (hunkMatch) { | ||
| currentLine = parseInt(hunkMatch[1], 10) - 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (line.startsWith('+') && !line.startsWith('+++')) { | ||
| currentLine++; | ||
| addedLines.add(currentLine); | ||
| } else if (!line.startsWith('-')) { | ||
| currentLine++; | ||
| } | ||
| } | ||
|
|
||
| return addedLines; | ||
| } | ||
|
|
||
| const changedFiles = getChangedJsFiles(); | ||
| if (changedFiles.length === 0) { | ||
| console.log('No changed JS files to check.'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| console.log(`Checking ${changedFiles.length} changed JS file(s) for async/await compliance...\n`); | ||
|
|
||
| const project = new Project({ | ||
| compilerOptions: { allowJs: true, noEmit: true }, | ||
| skipAddingFilesFromTsConfig: true, | ||
| }); | ||
|
|
||
| const filesToCheck = changedFiles.filter(f => | ||
| !f.startsWith('tests/') && | ||
| !f.startsWith('node_modules/') && | ||
| ( | ||
| f.startsWith('lib/') || | ||
| f.startsWith('bin/') || | ||
| !f.includes('/') | ||
| ) | ||
| ); | ||
| if (filesToCheck.length === 0) { | ||
| console.log('No source JS files in diff (tests and node_modules excluded).'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| project.addSourceFilesAtPaths(filesToCheck); | ||
|
|
||
| const violations = []; | ||
|
|
||
| for (const sourceFile of project.getSourceFiles()) { | ||
| const filePath = sourceFile.getFilePath().replace(process.cwd() + '/', ''); | ||
| const addedLines = getAddedLineNumbers(filePath); | ||
|
|
||
| if (addedLines.size === 0) continue; | ||
|
|
||
| const functions = [ | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionDeclaration), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionExpression), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.ArrowFunction), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.MethodDeclaration), | ||
| ]; | ||
|
|
||
| for (const fn of functions) { | ||
| if (fn.isAsync()) continue; | ||
|
|
||
| const startLine = fn.getStartLineNumber(); | ||
| if (!addedLines.has(startLine)) continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this the right filter? should we not search that there are no addedLines between
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good comment that I didn't notice. Anyway I suggest to keep current behaviour. We can start "slowly" and not too aggressive. This will allow dev to be familiar with async/await and introduce not too much change at the start. I'll add a comment in the guideline to setup your suggestion in some months ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| const params = fn.getParameters(); | ||
| const lastParam = params[params.length - 1]; | ||
| if (lastParam && CALLBACK_PARAM_PATTERN.test(lastParam.getName())) { | ||
| violations.push({ | ||
| file: filePath, | ||
| line: startLine, | ||
| type: 'callback', | ||
| detail: `function has callback parameter '${lastParam.getName()}'`, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (violations.length === 0) { | ||
| console.log('✓ All new code in the diff uses async/await.'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| console.error(`✗ Found ${violations.length} async/await violation(s) in the diff:\n`); | ||
| for (const v of violations) { | ||
| console.error(` ${v.file}:${v.line} [${v.type}] ${v.detail}`); | ||
| } | ||
| console.error('\nNew code must use async/await instead of callbacks.'); | ||
| console.error('See the async/await migration guide in CONTRIBUTING.md for help.'); | ||
| process.exit(1); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| /** | ||
| * Count async vs callback-style functions across the codebase using ts-morph. | ||
| * Used in CI to track async/await migration progress over time. | ||
| * | ||
| * Usage: node scripts/count-async-functions.mjs | ||
| */ | ||
| import { readFileSync } from 'node:fs'; | ||
| import { Project, SyntaxKind } from 'ts-morph'; | ||
|
|
||
| function getSourcePathsFromPackageJson() { | ||
| const packageJsonPath = new URL('../../package.json', import.meta.url); | ||
| const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | ||
| const paths = packageJson.countAsyncSourcePaths; | ||
|
|
||
| if (Array.isArray(paths) && paths.length > 0 && paths.every(p => typeof p === 'string')) { | ||
| return paths; | ||
| } | ||
|
|
||
| throw new Error('package.json must define a non-empty string array "countAsyncSourcePaths"'); | ||
| } | ||
|
|
||
| const project = new Project({ | ||
| compilerOptions: { | ||
| allowJs: true, | ||
| noEmit: true, | ||
| }, | ||
| skipAddingFilesFromTsConfig: true, | ||
| }); | ||
|
|
||
| project.addSourceFilesAtPaths(getSourcePathsFromPackageJson()); | ||
|
|
||
| let asyncFunctions = 0; | ||
| let totalFunctions = 0; | ||
| let callbackFunctions = 0; | ||
| let thenChains = 0; | ||
|
|
||
| const CALLBACK_PARAM_PATTERN = /^(cb|callback|next|done)$/i; | ||
|
|
||
| for (const sourceFile of project.getSourceFiles()) { | ||
| const functions = [ | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionDeclaration), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionExpression), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.ArrowFunction), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.MethodDeclaration), | ||
| ]; | ||
|
|
||
| for (const fn of functions) { | ||
| totalFunctions++; | ||
|
|
||
| if (fn.isAsync()) { | ||
| asyncFunctions++; | ||
| continue; | ||
| } | ||
|
|
||
| const params = fn.getParameters(); | ||
| const lastParam = params[params.length - 1]; | ||
| if (lastParam && CALLBACK_PARAM_PATTERN.test(lastParam.getName())) { | ||
| callbackFunctions++; | ||
| } | ||
| } | ||
|
|
||
| const propertyAccesses = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression); | ||
| for (const access of propertyAccesses) { | ||
| if (access.getName() === 'then') { | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| thenChains++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const asyncFunctionPercent = totalFunctions > 0 | ||
| ? ((asyncFunctions / totalFunctions) * 100).toFixed(1) | ||
| : '0.0'; | ||
|
|
||
| const migrationPercent = (asyncFunctions + callbackFunctions) > 0 | ||
| ? ((asyncFunctions / (asyncFunctions + callbackFunctions)) * 100).toFixed(1) | ||
| : '0.0'; | ||
|
|
||
| console.log('=== Async/Await Migration Progress ==='); | ||
| console.log(`Total functions: ${totalFunctions}`); | ||
| console.log(`Async functions: ${asyncFunctions} (${asyncFunctionPercent}%)`); | ||
| console.log(`Callback functions: ${callbackFunctions}`); | ||
| console.log(`Remaining .then(): ${thenChains}`); | ||
| console.log(''); | ||
| console.log(`Migration (trend): ${asyncFunctions}/${asyncFunctions + callbackFunctions} (${migrationPercent}%)`); | ||
|
|
||
| if (process.env.GITHUB_STEP_SUMMARY) { | ||
| const { appendFileSync } = await import('node:fs'); | ||
| appendFileSync(process.env.GITHUB_STEP_SUMMARY, [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we really want a summary in every PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO we can keep it simple as compute the report in each PR. Not a big deal, it's only some seconds so no finance impact.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not thinking about build time or finance, but really about relevance:
the summary does not hurt I agree, but I'd rather have something more dedicated to their purpose, like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point 🙏. Let's start simple again and I'll add your comment. We don't know how this will be use and what is needed from the team/dev. So I suggest to not waste time on that and iterate when we have feedback ? |
||
| '## Async/Await Migration Progress', | ||
| '', | ||
| `| Metric | Count |`, | ||
| `|--------|-------|`, | ||
| `| Total functions | ${totalFunctions} |`, | ||
| `| Async functions | ${asyncFunctions} (${asyncFunctionPercent}%) |`, | ||
| `| Callback-style functions | ${callbackFunctions} |`, | ||
| `| Remaining \`.then()\` chains | ${thenChains} |`, | ||
| `| Migration trend (async / (async + callback)) | ${asyncFunctions}/${asyncFunctions + callbackFunctions} (${migrationPercent}%) |`, | ||
| '', | ||
| ].join('\n')); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.