From 6a505158c128e4e447b4886c91b7fca1b7e1b9d8 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sun, 13 Sep 2026 19:51:54 +0100 Subject: [PATCH] feat(lint): make lint runnable, cover the shipped lib/, and run it in CI `npm run lint` now exits 0, lints the shipped JavaScript for the first time, and runs as its own CI job. Three separate problems, in increasing order of how much they mattered. 1. Nothing ran lint. The `lint` script existed but no workflow called it, so breaking it cost nothing - which is how a TypeScript major bump left typescript-eslint refusing to load for weeks without a single red build. Added a `lint` job to test.yml. It needs no SQL Server, no ODBC driver and no native build, so `npm ci --ignore-scripts` is enough and it finishes in under a minute. 2. The shipped code was never linted. eslint.config.mjs targets `lib/**/*.ts`, and the only .ts file in lib/ is index.d.ts, which the very same block then ignores via `ignores: ['**/*.d.ts']`. lib/ is 26 .js files. So the config looked like it covered the product code while covering none of it, and `npm run lint` only ever checked 11 files in test/ and samples/. Added a lib/**/*.js block with the same stylistic rules, and widened the lint scripts to include it. Stylistic rules only: eslint-config-love is type-aware and expects a tsconfig project, which plain .js files here cannot supply. The shipped code turned out to be in good shape - 4 violations, all whitespace, all auto-fixed. `git diff -w` on lib/ is empty, so nothing changed but layout. 3. The 75 pre-existing violations. 62 went with --fix. Of the rest: - 3 x no-promise-executor-return: `new Promise(resolve => setTimeout(...))` returns the timer handle out of the executor. Braced. - 1 x no-floating-promises: the async IIFE at the end of samples/typescript/logger-example.ts had no catch, so any failure in the sample became an unhandled rejection with a bare stack trace. Given a catch that logs and sets exitCode. - 11 x only-throw-error and 9 x no-non-null-assertion: turned off, with the reasons recorded in the config. The first cannot be satisfied without rewriting harness code that rethrows driver callback errors - QueryCb types err as `Error | undefined`, but the overloaded query() signature leaves it unresolvable at some call sites and cmd-test.ts declares several handlers as `err: any`. The second is `res![0]` in tests that just inserted the row they select. Both are consistent with the ~15 no-unsafe-* rules already off for the same reason: the driver's own surface is loosely typed. One autofix had to be reverted. `prefer-object-has-own` rewrote `Object.prototype.hasOwnProperty.call(o, k)` to `Object.hasOwn(o, k)`, which does not compile: tsconfig sets target es6 with no explicit lib, and Object.hasOwn is es2022, so tsc failed with TS2550. Rule disabled and the change reverted. Raising the target is the better fix, but it changes emitted dist/ output and the generated declarations, so it belongs in its own change rather than riding along with lint wiring. Verified: npm run lint exits 0; npm run build and npm run build:test both exit 0; the driver still loads; connection-pool suite 42 passing and query/connect suites 26 passing, which covers notifier.js and sql-client.js, the two lib files touched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K5wuZia58hUzEqN7CvHtz9 --- .github/workflows/test.yml | 35 +++++++++++++ eslint.config.mjs | 52 +++++++++++++++++++ lib/notifier.js | 2 +- lib/sql-client.js | 6 +-- package.json | 4 +- samples/typescript/logger-example.ts | 31 ++++++----- samples/typescript/pooling.ts | 2 +- samples/typescript/simple-logging-promises.ts | 28 +++++----- test/stress/stress-test-runner.ts | 48 ++++++++--------- 9 files changed, 150 insertions(+), 58 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3eba3ff1..77c60833 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,6 +8,41 @@ on: workflow_dispatch: jobs: + # Lint runs as its own job, first and cheapest. It needs no SQL Server, no ODBC + # driver and no native build, so `npm ci --ignore-scripts` is enough and it + # finishes in well under a minute. + # + # It exists because `npm run lint` was broken for weeks without anyone + # noticing: the script was never wired into CI, so a TypeScript major bump that + # made typescript-eslint refuse to load cost nothing and went unseen. Lint that + # nothing runs is lint that rots. + lint: + name: Lint + runs-on: ubuntu-latest + + steps: + - name: Create directory structure + run: mkdir -p node_modules + + - uses: actions/checkout@v7 + with: + path: node_modules/msnodesqlv8 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version: 22 + + # --ignore-scripts skips the native build: eslint never loads the addon, and + # building it here would add minutes for no benefit. + - name: Install dependencies + working-directory: node_modules/msnodesqlv8 + run: npm ci --ignore-scripts + + - name: Lint + working-directory: node_modules/msnodesqlv8 + run: npm run lint + test-linux: name: Test Linux - Node ${{ matrix.node }} strategy: diff --git a/eslint.config.mjs b/eslint.config.mjs index 03c0c4d0..e0ca6424 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -85,12 +85,31 @@ export default [ '@typescript-eslint/no-empty-object-type': 'off', '@typescript-eslint/no-empty-function': 'off', '@typescript-eslint/no-deprecated': 'off', + // Harness and sample code rethrows errors straight from driver + // callbacks. QueryCb types err as `Error | undefined`, but the + // overloaded query(sqlOrQuery, paramsOrCb?, cb?) signature leaves it + // unresolvable at some call sites, and test/env/cmd-test.ts declares + // several of its own handlers as `err: any`. The rule then cannot + // prove the thrown value is an Error even where a `!= null` guard + // precedes it. Off for the same reason the no-unsafe-* rules above + // are off - the driver's own surface is loosely typed. + '@typescript-eslint/only-throw-error': 'off', + // `res![0]` in a test that has just inserted the row it selects. + // Consistent with consistent-type-assertions being off above. + '@typescript-eslint/no-non-null-assertion': 'off', '@typescript-eslint/no-unnecessary-type-arguments': 'off', '@typescript-eslint/no-unnecessary-type-parameters': 'off', '@typescript-eslint/no-unnecessary-type-conversion': 'off', '@typescript-eslint/no-base-to-string': 'off', '@typescript-eslint/prefer-optional-chain': 'off', '@typescript-eslint/prefer-for-of': 'off', + // Its autofix rewrites Object.prototype.hasOwnProperty.call(o, k) to + // Object.hasOwn(o, k), which does not compile here: tsconfig sets + // target es6 with no explicit lib, and Object.hasOwn is es2022, so tsc + // fails with TS2550. Raising the target is the real fix but it changes + // emitted dist/ output and the generated declarations, so it wants its + // own change rather than riding along with lint wiring. + 'prefer-object-has-own': 'off', 'no-plusplus': 'off', 'no-console': 'off', 'no-param-reassign': 'off', @@ -119,6 +138,39 @@ export default [ '@eslint-community/eslint-comments/no-unlimited-disable': 'off' } }, + { + // lib/ is the shipped JavaScript and had NO lint coverage at all: the block + // above targets lib/**/*.ts, and the only .ts in lib is index.d.ts, which + // the same block then ignores. So 26 product files were never linted. + // + // Stylistic rules only. eslint-config-love is type-aware and expects a + // tsconfig project; pointing it at plain .js files would need type info + // these files do not have. This at least holds the shipped code to the same + // formatting as everything else. + files: ['lib/**/*.js'], + plugins: { '@stylistic': stylistic }, + rules: { + '@stylistic/semi': ['error', 'never'], + '@stylistic/no-trailing-spaces': 'error', + '@stylistic/eol-last': ['error', 'always'], + '@stylistic/no-multiple-empty-lines': ['error', { max: 1, maxEOF: 0 }], + '@stylistic/quotes': ['error', 'single', { avoidEscape: true }], + '@stylistic/comma-dangle': ['error', 'never'], + '@stylistic/space-before-function-paren': ['error', 'always'], + '@stylistic/keyword-spacing': 'error', + '@stylistic/space-infix-ops': 'error', + '@stylistic/comma-spacing': 'error', + '@stylistic/brace-style': ['error', '1tbs', { allowSingleLine: true }], + '@stylistic/block-spacing': 'error', + '@stylistic/key-spacing': 'error', + '@stylistic/space-before-blocks': 'error', + '@stylistic/no-multi-spaces': 'error', + '@stylistic/object-curly-spacing': ['error', 'always'], + '@stylistic/array-bracket-spacing': ['error', 'never'], + '@stylistic/spaced-comment': ['error', 'always'], + '@stylistic/indent': ['error', 2] + } + }, { ignores: ['dist/**', 'node_modules/**'] } diff --git a/lib/notifier.js b/lib/notifier.js index b14503e7..f7a7cd9a 100755 --- a/lib/notifier.js +++ b/lib/notifier.js @@ -222,7 +222,7 @@ const notifyModule = ((() => { if (this.timeoutHandle) { clearTimeout(this.timeoutHandle) } - + if (timeoutMs && timeoutMs > 0) { this.timeoutHandle = setTimeout(() => { logger.debugLazy(() => `Timeout ${timeoutMs}ms reached for query ${this.queryId} - triggering cancel`) diff --git a/lib/sql-client.js b/lib/sql-client.js index 424912fe..4d3da08b 100755 --- a/lib/sql-client.js +++ b/lib/sql-client.js @@ -114,9 +114,9 @@ class SqlClient { getConnectObject (p) { return typeof p === 'string' ? { - conn_str: p, - connect_timeout: 0 - } + conn_str: p, + connect_timeout: 0 + } : p } diff --git a/package.json b/package.json index 968cdac7..db630413 100644 --- a/package.json +++ b/package.json @@ -96,8 +96,8 @@ "test:cpp": "npm run configure:cpp-tests && npm run build:cpp-tests && cd test/cpp/build && ctest -C Release -V", "test:cpp:debug": "npm run configure:cpp-tests && cmake --build test/cpp/build --config Debug -j 8 && cd test/cpp/build && ctest -C Debug -V", "test:cpp:debug:exe": "test/cpp/build/cpp_tests --gtest_filter=\"*\"", - "lint": "eslint \"test/**/*.ts\" \"samples/**/*.ts\"", - "lint:fix": "eslint \"test/**/*.ts\" \"samples/**/*.ts\" --fix", + "lint": "eslint \"lib/**/*.js\" \"test/**/*.ts\" \"samples/**/*.ts\"", + "lint:fix": "eslint \"lib/**/*.js\" \"test/**/*.ts\" \"samples/**/*.ts\" --fix", "test:cpp:clean": "rimraf test/cpp/build && npm run configure:cpp-tests && cmake --build test/cpp/build --config Debug -j 8", "test:cpp:clean:vs": "rimraf test/cpp/build && npm run configure:cpp-tests:vs && cmake --build test/cpp/build --config Debug -j 8", "build:debug": "node-gyp rebuild --debug --verbose -j 4", diff --git a/samples/typescript/logger-example.ts b/samples/typescript/logger-example.ts index e0a23faa..7d9a36f0 100644 --- a/samples/typescript/logger-example.ts +++ b/samples/typescript/logger-example.ts @@ -17,26 +17,26 @@ sql.logger.configureForDevelopment() // Replace with your connection string const connectionString = 'Driver={ODBC Driver 18 for SQL Server};Server=127.0.0.1,1433;Database=node;UID=node_user;PWD=StrongPassword123!;TrustServerCertificate=yes;;Connect Timeout=10' -async function main(): Promise { +async function main (): Promise { let connection try { // The logger will automatically log connection attempts connection = await sql.promises.open(connectionString) - + // Simple query const result: QueryAggregatorResults = await connection.promises.query( 'SELECT name, create_date FROM sys.tables WHERE type = ?', - ['U'] // 'U' for user tables + ['U'] // 'U' for user tables ) - + console.log(`Found ${result.rows} tables`) console.log('First few tables:', result.first?.slice(0, 3)) - + // Stored procedure call (if you have one) // const procResult = await connection.promises.callProc('sp_who') // console.log('Active connections:', procResult.first?.length) - + } catch (error) { // Errors are automatically logged at ERROR level console.error('Database operation failed:', error) @@ -48,7 +48,7 @@ async function main(): Promise { } // Using connection pool with logging -async function poolExample(): Promise { +async function poolExample (): Promise { const pool = new sql.Pool({ connectionString, floor: 2, @@ -57,16 +57,16 @@ async function poolExample(): Promise { try { await pool.promises.open() - + // Execute multiple queries - the pool and logger will show which connection handles each const results = await Promise.all([ pool.promises.query('SELECT 1 as num'), pool.promises.query('SELECT 2 as num'), pool.promises.query('SELECT 3 as num') ]) - + console.log('Pool query results:', results.map(r => r.first?.[0])) - + } finally { await pool.promises.close() } @@ -76,11 +76,16 @@ async function poolExample(): Promise { (async () => { console.log('=== Single Connection Example ===') await main() - + console.log('\n=== Connection Pool Example ===') await poolExample() - + // Get final logger stats const config = sql.logger.getConfiguration() console.log('\nLogger configuration:', config) -})() \ No newline at end of file +})().catch(e => { + // Without this the IIFE is a floating promise: any failure above becomes + // an unhandled rejection, exiting with a bare stack trace and no context. + console.error(e) + process.exitCode = 1 +}) diff --git a/samples/typescript/pooling.ts b/samples/typescript/pooling.ts index 78f3dd45..38b99773 100755 --- a/samples/typescript/pooling.ts +++ b/samples/typescript/pooling.ts @@ -34,7 +34,7 @@ function submit (sql: string): Query { console.log(`send ${timeStr}, sql = ${sql}`) q.on('submitted', (d: QueryDescription) => { console.log(`query submitted ${timeStr}, sql = ${d.query_str}`) - q.on('done', () => console.log(`query done ${timeStr}`)) + q.on('done', () => { console.log(`query done ${timeStr}`) }) }) return q } diff --git a/samples/typescript/simple-logging-promises.ts b/samples/typescript/simple-logging-promises.ts index 1b31fbee..51b729b3 100644 --- a/samples/typescript/simple-logging-promises.ts +++ b/samples/typescript/simple-logging-promises.ts @@ -27,30 +27,30 @@ console.log('Current logger configuration:', config) // Example connection string - replace with your own const connectionString = 'Driver={ODBC Driver 18 for SQL Server};Server=127.0.0.1,1433;Database=node;UID=node_user;PWD=StrongPassword123!;TrustServerCertificate=yes;;Connect Timeout=10' -async function demonstrateLogging(): Promise { +async function demonstrateLogging (): Promise { try { // Log custom messages at different levels sql.logger.trace('Starting promise-based SQL operations', 'demonstrateLogging') sql.logger.debug('Connection string configured', 'demonstrateLogging') - + // Open connection using promises sql.logger.info('Opening database connection...', 'demonstrateLogging') const connection = await sql.promises.open(connectionString) sql.logger.info('Connection opened successfully', 'demonstrateLogging') - + // Execute a simple query sql.logger.debug('Executing test query', 'demonstrateLogging') const results: QueryAggregatorResults = await connection.promises.query('SELECT 1 as test, GETDATE() as currentTime') - + sql.logger.info(`Query completed in ${results.elapsed}ms`, 'demonstrateLogging') sql.logger.debug(`Results: ${JSON.stringify(results.first)}`, 'demonstrateLogging') - + // Demonstrate lazy logging for expensive operations sql.logger.traceLazy(() => { // This function is only called if TRACE level is enabled return `Detailed results: ${JSON.stringify(results, null, 2)}` }, 'demonstrateLogging') - + // Execute multiple queries using pool sql.logger.info('Creating connection pool...', 'demonstrateLogging') const pool = new sql.Pool({ @@ -61,10 +61,10 @@ async function demonstrateLogging(): Promise { heartbeatSql: 'SELECT @@SPID as spid', inactivityTimeoutSecs: 60 }) - + await pool.promises.open() sql.logger.info('Pool opened successfully', 'demonstrateLogging') - + // Execute queries in parallel sql.logger.debug('Executing parallel queries', 'demonstrateLogging') const parallelQueries = Promise.all([ @@ -72,19 +72,19 @@ async function demonstrateLogging(): Promise { pool.promises.query('SELECT 2 as query2'), pool.promises.query('SELECT 3 as query3') ]) - + const parallelResults = await parallelQueries sql.logger.info(`Completed ${parallelResults.length} parallel queries`, 'demonstrateLogging') - + // Clean up sql.logger.debug('Closing pool', 'demonstrateLogging') await pool.promises.close() - + sql.logger.debug('Closing connection', 'demonstrateLogging') await connection.promises.close() - + sql.logger.info('All operations completed successfully', 'demonstrateLogging') - + } catch (error) { sql.logger.error(`Operation failed: ${error}`, 'demonstrateLogging') throw error @@ -103,4 +103,4 @@ demonstrateLogging() .catch((error) => { console.error('Logging demonstration failed:', error) process.exit(1) - }) \ No newline at end of file + }) diff --git a/test/stress/stress-test-runner.ts b/test/stress/stress-test-runner.ts index be1c3f7d..c7b50a59 100644 --- a/test/stress/stress-test-runner.ts +++ b/test/stress/stress-test-runner.ts @@ -27,14 +27,14 @@ interface TestScenario { } class StressTestRunner { - private config: Required + private readonly config: Required private startTime: number | null = null private memoryBaseline: MemoryUsage | null = null private iterationCount = 0 private errors: TestError[] = [] private connection: any = null - constructor(config: StressTestConfig = {}) { + constructor (config: StressTestConfig = {}) { this.config = { connectionString: config.connectionString || process.env.CONNECTION_STRING || '', iterations: config.iterations || 10000, @@ -43,7 +43,7 @@ class StressTestRunner { } } - private getMemoryUsage(): MemoryUsage { + private getMemoryUsage (): MemoryUsage { const usage = process.memoryUsage() return { rss: (usage.rss / 1024 / 1024).toFixed(2), @@ -54,7 +54,7 @@ class StressTestRunner { } } - private logMemory(message = ''): void { + private logMemory (message = ''): void { const memory = this.getMemoryUsage() console.log(`[Memory] ${message}`) console.log(` RSS: ${memory.rss} MB`) @@ -62,7 +62,7 @@ class StressTestRunner { console.log(` Heap Used: ${memory.heapUsed} MB`) console.log(` External: ${memory.external} MB`) console.log(` Array Buffers: ${memory.arrayBuffers} MB`) - + if (this.memoryBaseline) { const heapDelta = (parseFloat(memory.heapUsed) - parseFloat(this.memoryBaseline.heapUsed)).toFixed(2) const rssDelta = (parseFloat(memory.rss) - parseFloat(this.memoryBaseline.rss)).toFixed(2) @@ -72,7 +72,7 @@ class StressTestRunner { console.log('') } - private async connect(): Promise { + private async connect (): Promise { try { this.connection = await sql.promises.open(this.config.connectionString) console.log('Connected to database') @@ -82,7 +82,7 @@ class StressTestRunner { } } - private async disconnect(): Promise { + private async disconnect (): Promise { if (this.connection) { try { await this.connection.promises.close() @@ -93,7 +93,7 @@ class StressTestRunner { } } - private async setupTestTable(): Promise { + private async setupTestTable (): Promise { try { // Drop table if exists await this.connection.promises.query(` @@ -145,7 +145,7 @@ class StressTestRunner { } } - private async cleanupTestTable(): Promise { + private async cleanupTestTable (): Promise { try { await this.connection.promises.query(` IF OBJECT_ID('dbo.stress_test_table', 'U') IS NOT NULL @@ -157,7 +157,7 @@ class StressTestRunner { } } - private async runScenario(scenario: TestScenario): Promise { + private async runScenario (scenario: TestScenario): Promise { console.log(`\nRunning scenario: ${scenario.name}`) console.log(`Description: ${scenario.description}`) console.log(`Iterations: ${this.config.iterations}`) @@ -166,11 +166,11 @@ class StressTestRunner { this.startTime = Date.now() this.iterationCount = 0 this.errors = [] - + // Force GC and record baseline memory if ((global as any).gc) { (global as any).gc() - await new Promise(resolve => setTimeout(resolve, 100)) + await new Promise(resolve => { setTimeout(resolve, 100) }) } this.memoryBaseline = this.getMemoryUsage() this.logMemory('Baseline') @@ -192,7 +192,7 @@ class StressTestRunner { // Force GC periodically if ((global as any).gc && i > 0 && i % this.config.gcInterval === 0) { (global as any).gc() - await new Promise(resolve => setTimeout(resolve, 10)) + await new Promise(resolve => { setTimeout(resolve, 10) }) } } catch (err) { @@ -205,7 +205,7 @@ class StressTestRunner { } // Final report - const totalTime = (Date.now() - this.startTime!) / 1000 + const totalTime = (Date.now() - this.startTime) / 1000 const rate = this.iterationCount / totalTime console.log('\n----------------------------------------') @@ -214,7 +214,7 @@ class StressTestRunner { console.log(`Total time: ${totalTime.toFixed(2)} seconds`) console.log(`Average rate: ${rate.toFixed(1)} operations/second`) console.log(`Errors: ${this.errors.length}`) - + if (this.errors.length > 0) { console.log('\nFirst few errors:') this.errors.slice(0, 3).forEach(({ iteration, error }) => { @@ -225,12 +225,12 @@ class StressTestRunner { // Force GC and final memory report if ((global as any).gc) { (global as any).gc() - await new Promise(resolve => setTimeout(resolve, 100)) + await new Promise(resolve => { setTimeout(resolve, 100) }) } this.logMemory('Final') } - async run(scenarioName: string): Promise { + async run (scenarioName: string): Promise { try { await this.connect() await this.setupTestTable() @@ -253,7 +253,7 @@ class StressTestRunner { } // Define stress test scenarios - private scenarios: Record = { + private readonly scenarios: Record = { 'string-select': { name: 'String Column Selection', description: 'Repeatedly select string columns to check for memory leaks', @@ -330,14 +330,14 @@ class StressTestRunner { return new Promise((resolve, reject) => { const query = conn.query('SELECT * FROM stress_test_table') let rowCount = 0 - + query.on('row', (row: any) => { rowCount++ // Access the data const _ = row }) - - query.on('done', () => resolve()) + + query.on('done', () => { resolve() }) query.on('error', reject) }) } @@ -360,7 +360,7 @@ class StressTestRunner { // CLI interface if (require.main === module) { const args = process.argv.slice(2) - + if (args.length === 0) { console.log('Usage: node stress-test-runner.js [options]') console.log('\nAvailable scenarios:') @@ -389,7 +389,7 @@ if (require.main === module) { for (let i = 1; i < args.length; i += 2) { const option = args[i] const value = args[i + 1] - + switch (option) { case '--iterations': config.iterations = parseInt(value, 10) @@ -429,4 +429,4 @@ if (require.main === module) { }) } -export = StressTestRunner \ No newline at end of file +export = StressTestRunner