Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions .github/scripts/check-diff-async.mjs
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;

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the right filter?
This would consider only functions whose "header" is changed : and skip if only an update is made inside the existing function's body...

should we not search that there are no addedLines between fn.getStartLineNumber() and fn.getEndLineNumber() (or however it is called)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
100 changes: 100 additions & 0 deletions .github/scripts/count-async-functions.mjs
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') {
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, [
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really want a summary in every PR?
for monitoring the progress, this may be done in a separate job (eg. nightly on on dev branches) which builds a report giving the status on the "latest branch" (as a badge in the README, as a GitHub page, ....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • a summary is "inside" the build, so people will not go look at it - not will it show them how much progress they made (in their PR)
  • for "monitoring", i.e. if someone want to (periodically...) check the status : not sure it is very practical to just go and look inside the last finished build

the summary does not hurt I agree, but I'd rather have something more dedicated to their purpose, like

  • a report in a "static" place : GitHub page, badge in the Readme, periodic reporting job...
  • a comment on the PR indicating how this specific PR affected the async migration (like "Great, you removed 10 async calls! Migration is now 53% complete, keep pushing!")

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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'));
}
23 changes: 21 additions & 2 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: actions/setup-node@v4
with:
node-version: '22'
Expand All @@ -95,14 +97,31 @@ jobs:
cache: pip
- name: Install python deps
run: pip install flake8
- name: Lint Javascript
run: yarn run --silent lint -- --max-warnings 0
- name: Lint Javascript (strict, excluding async migration rules)
run: |
yarn run --silent lint -- --max-warnings 0 --rule "promise/prefer-await-to-then: off" --rule "n/callback-return: off"
- name: Lint Markdown
run: yarn run --silent lint_md
- name: Lint python
run: flake8 $(git ls-files "*.py")
- name: Lint Yaml
run: yamllint -c yamllint.yml $(git ls-files "*.yml")
- name: Check async/await compliance in diff
run: yarn run check-diff-async

async-migration-report:
runs-on: ubuntu-24.04
steps:
- name: Checkout
uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: '22'
cache: yarn
- name: install dependencies
run: yarn install --frozen-lockfile --network-concurrency 1
- name: Count async/await migration progress
run: yarn run count-async

unit-tests:
runs-on: ubuntu-24.04
Expand Down
6 changes: 6 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import mocha from "eslint-plugin-mocha";
import promise from "eslint-plugin-promise";
import n from "eslint-plugin-n";
import path from "node:path";
import { fileURLToPath } from "node:url";
import js from "@eslint/js";
Expand All @@ -15,6 +17,8 @@ const compat = new FlatCompat({
export default [...compat.extends('@scality/scality'), {
plugins: {
mocha,
promise,
n,
},

languageOptions: {
Expand Down Expand Up @@ -67,5 +71,7 @@ export default [...compat.extends('@scality/scality'), {
"quote-props": "off",
"mocha/no-exclusive-tests": "error",
"no-redeclare": ["error", { "builtinGlobals": false }],
"promise/prefer-await-to-then": "warn",
"n/callback-return": "warn",
},
}];
15 changes: 14 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
"eslint": "^9.14.0",
"eslint-plugin-import": "^2.31.0",
"eslint-plugin-mocha": "^10.5.0",
"eslint-plugin-n": "^17.24.0",
"eslint-plugin-promise": "^7.2.1",
"express": "^4.21.1",
"ioredis": "^5.4.1",
"istanbul": "^0.4.5",
Expand All @@ -79,12 +81,21 @@
"nyc": "^15.1.0",
"pino-pretty": "^13.1.3",
"sinon": "^13.0.1",
"ts-morph": "^27.0.2",
"tv4": "^1.3.0"
},
"resolutions": {
"jsonwebtoken": "^9.0.0",
"nan": "v2.22.0"
},
"countAsyncSourcePaths": [
"lib/**/*.js",
"index.js",
"dataserver.js",
"mdserver.js",
"managementAgent.js",
"bin/**/*.js"
],
"mocha": {
"recursive": true,
"timeout": 40000
Expand Down Expand Up @@ -141,6 +152,8 @@
"test_sur": "mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json --recursive tests/sur --exit",
"multiple_backend_test": "CI=true S3BACKEND=mem S3METADATA=mem S3DATA=multiple mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json -t 20000 --recursive tests/multipleBackend --exit",
"cover": "nyc --clean --silent yarn run",
"postcover": "nyc report --report-dir ./coverage/test --reporter=lcov"
"postcover": "nyc report --report-dir ./coverage/test --reporter=lcov",
"count-async": "node .github/scripts/count-async-functions.mjs",
"check-diff-async": "node .github/scripts/check-diff-async.mjs"
}
}
Loading
Loading