fix: add stricter type-checking and improve event loop handling #11540
Conversation
## Walkthrough
This update refactors a wide range of files to improve type safety, modernize asynchronous handling, and enhance code clarity. Key changes include converting many asynchronous functions and test cases to synchronous patterns where appropriate, updating type annotations for improved precision, refining ESLint and package configurations, and cleaning up test assertions and import organization. No major runtime logic is altered.
## Changes
| Files/Groups | Change Summary |
|-----------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------|
| `eslint.config.mjs`, `package.json` | Updated ESLint config for clarity and stricter rules; upgraded and fixed devDependencies and exports in package. |
| `sample/sample*-entity/app.ts` | Refactored from promise chains to `async/await` with improved error handling and resource cleanup. |
| `src/cli.ts`, `src/query-builder/RelationLoader.ts`, `src/query-runner/QueryLock.ts` | Updated promise handling and added/suppressed ESLint directives for floating promises. |
| `src/connection/*`, `src/driver/*`, `src/entity-manager/MongoEntityManager.ts` | Refactored async methods to synchronous where possible; improved type annotations and removed unnecessary awaits. |
| `src/metadata/ColumnMetadata.ts`, `src/logger/FormattedConsoleLogger.ts` | Minor syntax cleanups and const/let usage improvements. |
| `src/persistence/SubjectExecutor.ts`, `src/util/ImportUtils.ts`, `src/util/OrmUtils.ts` | Improved typings, refactored deep merge/clone logic, and simplified async handling. |
| `src/driver/mongodb/bson.typings.ts`, `src/driver/mongodb/typings.ts` | Removed internal BSON namespace exports; converted `AutoEncrypter` interface to class; cleaned imports/exports. |
| `test/functional/*`, `test/github-issues/*`, `test/other-issues/*`, `test/unit/*` | Modernized test syntax, replaced `should` with `expect`, removed unnecessary async/await, improved assertions. |
| `test/github-issues/1716/entity/mariadbEntity.ts` | Deleted unused MariaDB entity class. |
| `test/utils/xfail.ts` | Updated wrapper function to return correct async type. |
| `test/github-issues/3534/entity/RegExpStringTransformer.ts` | Refactored namespace of RegExpStringTransformer to a constant object with methods. |
| Various test files | Replaced Promise.all with synchronous loops; removed redundant async/await; improved assertion styles and imports.|
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant TestRunner
participant Connection
participant Repository
TestRunner->>Connection: getRepository(Entity) (sync)
TestRunner->>Repository: perform operation (e.g., save, find)
Repository-->>TestRunner: result (Promise or value)
Note over TestRunner: Assertions performed with expect stylesequenceDiagram
participant App
participant DataSource
App->>DataSource: initialize (await)
alt Success
App->>DataSource: perform operations (await)
App->>DataSource: destroy (await)
else Failure
App->>App: Log error
end
Possibly related PRs
Suggested reviewers
Poem
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/functional/columns/value-transformer/value-transformer.ts (2)
87-87: Verify that getRepository() is synchronous.Similar to the previous occurrence, confirm that removing
awaitfromconnection.getRepository()is correct.
106-106: Verify that getRepository() is synchronous.Confirm that removing
awaitfromconnection.getRepository()is correct for this occurrence as well.
🧹 Nitpick comments (8)
test/github-issues/1369/issue-1369.ts (1)
42-48: Consider adding null check for better test diagnostics.While the direct assertion simplifies the code, removing the null check for
foundEntitycould make test failures less informative. If the entity lookup fails, the test will now throw a different error than intended.Consider adding a null check for better test diagnostics:
const foundEntity = await connection.manager.findOne( ConcreteEntity, { where: { id: 1 }, }, ) +expect(foundEntity).to.not.be.null expect(foundEntity).to.deep.equal({ // ... rest of assertion })test/github-issues/587/issue-587.ts (1)
22-41: Consider the performance impact of sequential execution.The refactor from
Promise.alltoforEachchanges the test execution from parallel to concurrent sequential execution. While functionally correct, this may slow down test execution since each connection could be tested independently in parallel.Consider whether parallel execution is beneficial here, especially if this pattern differs from similar test files in the codebase.
test/github-issues/9318/issue-9318.ts (1)
27-36: Performance regression from parallel to sequential execution.Similar to other test files in this PR, this changes from parallel (
Promise.all) to sequential (forEach) execution. This pattern is inconsistent across the PR and may negatively impact test performance.For independent connection tests, parallel execution is typically preferable. Consider maintaining consistency with the async execution pattern chosen for similar test files.
eslint.config.mjs (2)
60-65: Consider re-enabling unsafe rules gradually.Multiple
@typescript-eslint/no-unsafe-*rules are disabled, which reduces the benefit of the stricter type checking configuration. While this is understandable for a large codebase migration, consider creating a plan to gradually re-enable these rules to improve type safety.
75-75: Temporary exception should be tracked.The
@typescript-eslint/no-unnecessary-type-assertionrule is marked as a "temporary exception". Consider creating a tracking issue to re-enable this rule once the unnecessary type assertions are cleaned up.Would you like me to help create a tracking issue or generate a script to identify unnecessary type assertions in the codebase?
test/github-issues/8832/issue-8832.ts (1)
175-201: Consider granularity trade-off in consolidated entity validation tests.While consolidating the three entity validation tests into a single test improves efficiency, it reduces debugging granularity. If one entity validation fails, it may be harder to identify which specific entity caused the issue.
Consider adding descriptive error messages or splitting into separate
itblocks if debugging becomes problematic in the future.src/util/OrmUtils.ts (2)
118-131: Good modernization with rest parametersUsing generic rest parameters
...args: T[]instead of theargumentsobject improves type safety and code clarity.Note: Static analysis flagged the use of
thisin the static context (line 131). While the current usage is functionally correct, consider usingOrmUtils.compare2Objectsfor consistency with static method conventions.
557-567: Type safety improvements with minor style suggestionsThe updated signature with
DeepPartial<T>and explicitvoidreturn type improves type safety. UsingObject.entriesis more idiomatic thanfor...inloops.Consider addressing the static analysis hints by replacing
thiswithOrmUtilsin static method calls (lines 562, 567) for better clarity:-if (this.isPlainObject(target) && this.isPlainObject(source)) { +if (OrmUtils.isPlainObject(target) && OrmUtils.isPlainObject(source)) {-this.mergeObjectKey(target, key, value, memo) +OrmUtils.mergeObjectKey(target, key, value, memo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (89)
eslint.config.mjs(3 hunks)package.json(5 hunks)sample/sample1-simple-entity/app.ts(1 hunks)sample/sample16-indexes/app.ts(1 hunks)sample/sample17-versioning/app.ts(1 hunks)src/cli.ts(2 hunks)src/connection/ConnectionOptionsReader.ts(1 hunks)src/connection/options-reader/ConnectionOptionsEnvReader.ts(1 hunks)src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts(1 hunks)src/driver/better-sqlite3/BetterSqlite3Driver.ts(2 hunks)src/driver/cordova/CordovaQueryRunner.ts(1 hunks)src/driver/expo/legacy/ExpoLegacyQueryRunner.ts(1 hunks)src/driver/mysql/MysqlQueryRunner.ts(1 hunks)src/driver/nativescript/NativescriptQueryRunner.ts(1 hunks)src/driver/react-native/ReactNativeQueryRunner.ts(1 hunks)src/driver/sap/SapQueryRunner.ts(1 hunks)src/driver/sqlite-abstract/AbstractSqliteDriver.ts(1 hunks)src/driver/sqlite/SqliteDriver.ts(1 hunks)src/entity-manager/MongoEntityManager.ts(3 hunks)src/logger/FormattedConsoleLogger.ts(1 hunks)src/metadata/ColumnMetadata.ts(1 hunks)src/persistence/SubjectExecutor.ts(2 hunks)src/query-builder/RelationLoader.ts(2 hunks)src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts(2 hunks)src/query-runner/QueryLock.ts(2 hunks)src/util/ImportUtils.ts(2 hunks)src/util/OrmUtils.ts(8 hunks)test/functional/columns/value-transformer/value-transformer.ts(2 hunks)test/functional/connection/connection.ts(13 hunks)test/functional/custom-repository/custom-repository.test.ts(1 hunks)test/functional/query-builder/brackets/query-builder-brackets.ts(1 hunks)test/functional/query-builder/cte/materialized-cte.ts(4 hunks)test/functional/query-builder/not/query-builder-not.ts(2 hunks)test/functional/query-builder/subquery/query-builder-subquery.ts(5 hunks)test/functional/query-builder/time-travel-query/time-travel-query.ts(5 hunks)test/functional/sql-tag/entity/PostgresExample.ts(1 hunks)test/functional/table-inheritance/single-table/database-option-inherited/database-option-inherited.ts(1 hunks)test/functional/tree-tables/closure-table/closure-table-mysql.ts(1 hunks)test/github-issues/10569/issue-10569.ts(1 hunks)test/github-issues/10626/issue-10626.ts(2 hunks)test/github-issues/108/issue-108.ts(1 hunks)test/github-issues/11085/issue-11085.ts(1 hunks)test/github-issues/11085/subscriber/UserSubscriber.ts(1 hunks)test/github-issues/11440/issue-11440.ts(3 hunks)test/github-issues/1369/issue-1369.ts(1 hunks)test/github-issues/1369/subscriber/AbstractEntitySubscriber.ts(1 hunks)test/github-issues/1576/issue-1576.ts(1 hunks)test/github-issues/1716/entity/mariadbEntity.ts(0 hunks)test/github-issues/1716/issue-1716.ts(4 hunks)test/github-issues/1805/issue-1805.ts(1 hunks)test/github-issues/2216/issue-2216.ts(2 hunks)test/github-issues/2518/issue-2518.ts(1 hunks)test/github-issues/2965/index.ts(1 hunks)test/github-issues/2984/issue-2984.ts(1 hunks)test/github-issues/3118/issue-3118.ts(1 hunks)test/github-issues/3158/issue-3158.ts(1 hunks)test/github-issues/3256/subscriber/PostSubscriber.ts(1 hunks)test/github-issues/3443/issue-3443.ts(1 hunks)test/github-issues/3588/issue-3588.ts(1 hunks)test/github-issues/3857/issue-3857.ts(2 hunks)test/github-issues/4185/subscriber/ExtendedAfterLoadSubscriber.ts(1 hunks)test/github-issues/4185/subscriber/SimpleAfterLoadSubscriber.ts(1 hunks)test/github-issues/4220/issue-4220.ts(1 hunks)test/github-issues/4842/issue-4842.ts(1 hunks)test/github-issues/4958/issue-4958.ts(1 hunks)test/github-issues/4980/issue-4980.ts(1 hunks)test/github-issues/5067/issue-5067.ts(1 hunks)test/github-issues/5119/issue-5119.ts(1 hunks)test/github-issues/512/issue-512.ts(2 hunks)test/github-issues/521/issue-521.ts(1 hunks)test/github-issues/5444/issue-5444.ts(1 hunks)test/github-issues/5704/subscriber/CategorySubscriber.ts(1 hunks)test/github-issues/587/issue-587.ts(1 hunks)test/github-issues/6168/issue-6168.ts(1 hunks)test/github-issues/6977/issue-6977.ts(1 hunks)test/github-issues/7203/issue-7203.ts(1 hunks)test/github-issues/7932/issue-7932.ts(1 hunks)test/github-issues/799/issue-799.ts(1 hunks)test/github-issues/8026/issue-8026.ts(1 hunks)test/github-issues/8408/delete-orphans.ts(2 hunks)test/github-issues/8832/issue-8832.ts(6 hunks)test/github-issues/8892/issue-8892.ts(1 hunks)test/github-issues/8970/issue-8970.ts(1 hunks)test/github-issues/9049/issue-9049.ts(1 hunks)test/github-issues/9318/issue-9318.ts(1 hunks)test/other-issues/mongodb-entity-change-in-listeners/entity/Post.ts(1 hunks)test/other-issues/preventing-injection/preventing-injection.ts(1 hunks)test/unit/util/orm-utils.ts(2 hunks)test/utils/xfail.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/github-issues/1716/entity/mariadbEntity.ts
🧰 Additional context used
🧬 Code Graph Analysis (23)
test/functional/query-builder/brackets/query-builder-brackets.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/functional/table-inheritance/single-table/database-option-inherited/database-option-inherited.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
src/connection/ConnectionOptionsReader.ts (1)
src/connection/options-reader/ConnectionOptionsEnvReader.ts (1)
ConnectionOptionsEnvReader(12-153)
test/functional/columns/value-transformer/value-transformer.ts (2)
src/connection/ConnectionManager.ts (1)
connections(16-18)src/schema-builder/view/View.ts (1)
View(13-123)
test/github-issues/4958/issue-4958.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/2984/issue-2984.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/3118/issue-3118.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/4980/issue-4980.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/587/issue-587.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/108/issue-108.ts (2)
test/utils/test-utils.ts (1)
closeTestingConnections(487-495)src/connection/ConnectionManager.ts (1)
connections(16-18)
test/unit/util/orm-utils.ts (1)
src/common/DeepPartial.ts (1)
DeepPartial(4-14)
test/functional/tree-tables/closure-table/closure-table-mysql.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/7203/issue-7203.ts (2)
src/connection/ConnectionManager.ts (1)
connections(16-18)src/query-builder/QueryBuilder.ts (1)
comment(546-549)
test/github-issues/9318/issue-9318.ts (3)
src/connection/ConnectionManager.ts (1)
connections(16-18)src/driver/postgres/PostgresDriver.ts (1)
PostgresDriver(35-1608)src/driver/DriverUtils.ts (1)
DriverUtils(8-297)
test/other-issues/mongodb-entity-change-in-listeners/entity/Post.ts (1)
src/decorator/listeners/AfterLoad.ts (1)
AfterLoad(8-16)
test/github-issues/2216/issue-2216.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/10569/issue-10569.ts (1)
test/github-issues/10569/contract/create-user-contract.ts (1)
CreateUserContract(1-3)
test/github-issues/7932/issue-7932.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/6977/issue-6977.ts (1)
src/connection/ConnectionManager.ts (1)
connections(16-18)
test/github-issues/4185/subscriber/ExtendedAfterLoadSubscriber.ts (1)
src/subscriber/event/LoadEvent.ts (1)
LoadEvent(9-36)
test/github-issues/3256/subscriber/PostSubscriber.ts (1)
src/subscriber/event/UpdateEvent.ts (1)
UpdateEvent(12-58)
test/github-issues/1369/subscriber/AbstractEntitySubscriber.ts (1)
src/subscriber/event/UpdateEvent.ts (1)
UpdateEvent(12-58)
test/github-issues/1716/issue-1716.ts (1)
test/utils/test-utils.ts (3)
createTestingConnections(388-482)reloadTestingDatabases(500-505)closeTestingConnections(487-495)
🪛 Biome (1.9.4)
src/util/OrmUtils.ts
[error] 131-131: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 562-562: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 562-562: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 567-567: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (130)
src/logger/FormattedConsoleLogger.ts (1)
28-28: Enforce immutability for loop variable
Switching toconstin thefor...ofloop prevents accidental reassignments tomessageand aligns with the project's immutability conventions.src/driver/mysql/MysqlQueryRunner.ts (1)
211-211: LGTM! Good optimization removing unnecessaryasynckeyword.The callback function doesn't use
awaitanywhere and performs only synchronous operations (logging, error handling, result creation). This change reduces unnecessary overhead and improves code clarity while maintaining the same functionality.src/driver/cordova/CordovaQueryRunner.ts (1)
64-64: Good improvement: Removed unnecessaryasyncfrom Promise executor.Promise executor functions should be synchronous unless they specifically need to await asynchronous operations. This change correctly removes the unnecessary
asynckeyword, improving performance by avoiding redundant promise wrapping.src/driver/react-native/ReactNativeQueryRunner.ts (1)
126-126: Correctly removed unnecessaryasyncfrom error callback.The error callback only performs synchronous operations (logging, broadcasting, and promise rejection), so the
asynckeyword was unnecessary. This change improves performance and aligns with similar patterns in other driver implementations.src/driver/expo/legacy/ExpoLegacyQueryRunner.ts (1)
226-226: Good practice: Usingconstfor array that doesn't need reassignment.This change correctly uses
constinstead ofletsince the array reference never changes, only its contents are modified. This prevents accidental reassignment and follows modern JavaScript/TypeScript best practices.src/driver/nativescript/NativescriptQueryRunner.ts (1)
59-59: Proper removal of unnecessaryasyncfrom Promise executor.The Promise executor function only performs synchronous operations and doesn't require
await, so removing theasynckeyword is correct and improves performance by avoiding unnecessary promise overhead.src/driver/sqlite/SqliteDriver.ts (1)
217-219: Correctly changed from async to synchronous iteration.The change properly replaces
for await...ofwithfor...ofsinceObject.values()returns a regular array, not an async iterable. This improves performance while maintaining the correct async behavior for the actual asynchronous operations within the loop.src/driver/sqlite-abstract/AbstractSqliteDriver.ts (1)
916-916: LGTM: Interface alignment for async consistency.The change from synchronous to asynchronous method signature aligns the abstract interface with concrete SQLite driver implementations that require asynchronous database connection setup. This improves type safety across the SQLite driver hierarchy.
src/entity-manager/MongoEntityManager.ts (3)
1030-1030: LGTM: Performance optimization by removing unnecessary async wrapper.Removing the
asynckeyword is correct here since the function returns a promise directly without awaiting any intermediate operations. This eliminates unnecessary promise wrapping overhead.
1045-1045: LGTM: Consistent optimization pattern applied.Same optimization as the
toArraymethod - removing the unnecessaryasynckeyword for a function that returns a promise directly.
1180-1180: LGTM: Type annotation improvement.Removing the generic type parameter
<Entity>fromcreateEntityCursorallows TypeScript to infer the type correctly, improving type safety.src/driver/better-sqlite3/BetterSqlite3Driver.ts (2)
158-158: LGTM: Proper async handling for database preparation.Adding
awaitensures that any asynchronous database preparation completes before proceeding with the connection setup. This is essential for proper initialization order.
201-203: LGTM: Correct iteration pattern for synchronous values.Changing from
for await...oftofor...ofis correct sinceObject.values(this.attachedDatabases)returns a regular array, not an async iterable. The previous usage was unnecessary and potentially confusing.src/metadata/ColumnMetadata.ts (1)
572-572: LGTM! Type safety improvement.Removing the explicit
: anyreturn type annotation allows TypeScript to properly infer the return type, improving type safety and enabling better static analysis.test/functional/sql-tag/entity/PostgresExample.ts (1)
29-29: Excellent type safety improvement.Narrowing the type from
any | nulltostring[] | nullprovides better type safety, clearer intent, and improved developer experience for this JSONB column storing string arrays.test/functional/query-builder/time-travel-query/time-travel-query.ts (1)
35-35: Correct removal of unnecessaryawaitkeywords.The
getRepository()method returns synchronously, not a Promise, so theseawaitkeywords were unnecessary and incorrect. These fixes improve code accuracy and align with proper async/await usage patterns.Also applies to: 67-67, 99-99, 151-152, 211-212
package.json (3)
23-25: LGTM! Proper exports field structure.Moving the
"types"entry inside the"node"field before"import"and"require"follows Node.js conventions and removes duplication.
154-154: Good addition of reflect-metadata devDependency.Adding
reflect-metadataas a devDependency is appropriate for TypeScript decorator support and aligns with the stricter type-checking improvements.
111-111: [web_search]What TypeScript versions does [email protected] support?test/github-issues/3158/issue-3158.ts (1)
10-10: Correct removal of unnecessaryasynckeyword.The
describecallback doesn't perform async operations itself - the async work is properly handled in the lifecycle hooks. This change improves code clarity and follows testing best practices.test/github-issues/3588/issue-3588.ts (1)
10-10: Good simplification of the test suite declaration.Removing the
asynckeyword is correct since the describe callback doesn't perform any asynchronous operations. All async setup and teardown is properly handled in the hooks.test/github-issues/5119/issue-5119.ts (1)
65-65: Excellent improvement to resource cleanup.Switching from synchronous
close()toawait dataSource.destroy()ensures proper asynchronous cleanup of database connections and prevents potential resource leaks. This aligns well with the PR's objective of improving event loop handling.test/github-issues/8970/issue-8970.ts (1)
11-11: Proper removal of unnecessary async keyword.The describe callback doesn't perform any asynchronous operations, so removing the
asynckeyword improves clarity and correctness. All async operations are appropriately handled within the test functions.test/github-issues/5444/issue-5444.ts (1)
10-10: Correct conversion to synchronous test function.The test performs only synchronous operations (transformer creation, method calls, and assertions), so removing the
asynckeyword is appropriate and improves type accuracy.test/functional/custom-repository/custom-repository.test.ts (1)
43-45: Proper removal of unnecessary await.The
withRepository()method is synchronous, so removing theawaitkeyword is correct and improves type safety. This change aligns with the stricter type-checking objectives of the PR.test/github-issues/2965/index.ts (1)
29-30: LGTM: Correctly removed unnecessaryawaitfrom synchronouscreatemethod.The
repository.create()method is synchronous and returns the entity instance directly, so removing theawaitkeywords is the correct approach.test/github-issues/8892/issue-8892.ts (1)
12-12: LGTM: Correctly removed unnecessaryasyncfrom describe callback.The describe callback doesn't perform any direct asynchronous operations, so removing the
asynckeyword is appropriate. The async operations are properly contained within the individual test hooks.test/github-issues/11085/issue-11085.ts (1)
29-29: LGTM: Correctly removed unnecessaryawaitfrom synchronousgetRepositorymethod.The
connection.getRepository()method is synchronous and returns the repository instance directly, so removing theawaitkeyword is the correct approach.test/github-issues/1805/issue-1805.ts (1)
28-28: LGTM: Correctly removed unnecessaryawaitfrom synchronousgetRepositorymethod.The
connection.getRepository()method is synchronous and returns the repository instance directly, so removing theawaitkeyword is the correct approach.test/github-issues/4220/issue-4220.ts (1)
50-50: LGTM: Correctly removed unnecessaryawaitfrom synchronousgetRepositorymethod.The
connection.getRepository()method is synchronous and returns the repository instance directly, so removing theawaitkeyword is the correct approach.test/github-issues/11085/subscriber/UserSubscriber.ts (1)
9-9: LGTM: Correctly removed unnecessaryawait.The
getRepository()method is synchronous and returns a Repository instance directly, not a Promise. Removing theawaitkeyword here is the correct fix and aligns with the stricter type-checking objectives.test/github-issues/799/issue-799.ts (1)
16-20: Excellent improvement to resource cleanup.The changes properly handle asynchronous cleanup with:
- Optional chaining (
?.) to safely check initialization status- Awaiting
dataSource.destroy()for proper resource disposal- Converting to async function to handle the awaited cleanup
This prevents potential resource leaks and improves test reliability.
test/functional/table-inheritance/single-table/database-option-inherited/database-option-inherited.ts (1)
31-37: Good simplification of synchronous operations.Converting from
Promise.allwith async mapping to a synchronousforEachloop is appropriate here since:
- Entity metadata access is synchronous
- No async operations are performed within the iteration
- The test logic remains unchanged while reducing complexity
This aligns well with the PR's goal of improving event loop handling by eliminating unnecessary async patterns.
test/github-issues/6168/issue-6168.ts (2)
110-113: Good type assertion improvement.Replacing type assertion (
as Table) with definite assignment assertion (!) is more semantically correct here. The definite assignment assertion clearly indicates that we expect the table to exist after the find operation, while providing the same non-null guarantee.
115-115: Proper asynchronous resource cleanup.Adding
awaittoqueryRunner.release()ensures the query runner is properly disposed of before the test continues. This prevents potential resource leaks and improves test reliability.test/github-issues/10569/issue-10569.ts (1)
28-40: ```shell
#!/bin/bashLocate EntityManager.ts and inspect the create() method signature
set -e
FILE=$(fd EntityManager.ts | head -n1)
echo "Inspecting: $FILE"
grep -n -A3 -B2 "create<" "$FILE"</details> <details> <summary>test/github-issues/10626/issue-10626.ts (2)</summary> `27-30`: **LGTM: Correctly removed unnecessary `await` for synchronous method.** The removal of `await` before `dataSource.setOptions()` is appropriate since configuration setters are typically synchronous operations that modify internal state immediately. --- `42-45`: **LGTM: Correctly removed unnecessary `await` for synchronous method.** Consistent with the previous change, removing `await` from the second `setOptions()` call is correct for a synchronous configuration method. </details> <details> <summary>test/github-issues/4958/issue-4958.ts (1)</summary> `23-34`: **LGTM: Correctly converted async test to synchronous iteration.** The conversion from `Promise.all` with async callbacks to a synchronous `for...of` loop is appropriate here since: 1. `QueryBuilder.getSql()` is a synchronous method that returns the built SQL string immediately 2. The test logic remains unchanged - it still tests SQL generation for each connection 3. Synchronous iteration is simpler and more readable for this use case </details> <details> <summary>test/github-issues/5704/subscriber/CategorySubscriber.ts (1)</summary> `10-12`: **LGTM: Correctly simplified synchronous lifecycle hook.** The conversion of `afterLoad` from async to synchronous is appropriate since the method only performs a simple property assignment (`entity.addedProp = true`). TypeORM's lifecycle hooks support both synchronous and asynchronous implementations, and using the synchronous form is more efficient when no async operations are needed. </details> <details> <summary>src/query-runner/QueryLock.ts (2)</summary> `5-5`: **LGTM: Improved type precision for the release function.** The type annotation refinement from `Function` to `() => void` provides better type safety and clarity about the expected signature of the release function. --- `21-22`: **LGTM: Appropriate ESLint suppression for synchronous array operation.** The ESLint disable comment for `@typescript-eslint/no-floating-promises` is correctly applied here since `Array.splice()` is a synchronous method that doesn't return a Promise. The linter may be incorrectly flagging this as a floating promise, so the suppression is justified. </details> <details> <summary>src/connection/ConnectionOptionsReader.ts (1)</summary> `141-141`: **LGTM! Correctly aligned with synchronous method signature.** The removal of `await` is appropriate since `ConnectionOptionsEnvReader.read()` is now synchronous and returns `DataSourceOptions[]` directly rather than a Promise. </details> <details> <summary>test/github-issues/4185/subscriber/SimpleAfterLoadSubscriber.ts (1)</summary> `14-16`: **LGTM! Correctly simplified synchronous lifecycle method.** Removing `async` is appropriate since the method only performs a synchronous property assignment without any asynchronous operations. </details> <details> <summary>test/github-issues/2984/issue-2984.ts (1)</summary> `21-31`: **LGTM! Appropriately simplified test to synchronous iteration.** The conversion from `Promise.all` with async callbacks to a synchronous `forEach` loop is correct since the test only accesses synchronous properties (`entityMetadatas`) without performing any asynchronous operations. </details> <details> <summary>test/functional/query-builder/cte/materialized-cte.ts (5)</summary> `11-11`: **LGTM! Updated to use modern DataSource type.** The import change from `Connection` to `DataSource` aligns with TypeORM's current API. --- `14-14`: **LGTM! Type declaration updated consistently.** The type change from `Connection[]` to `DataSource[]` is consistent with the import update. --- `52-57`: **LGTM! Correctly removed unnecessary await.** The `createQueryBuilder()` method is synchronous and returns a QueryBuilder instance immediately, so removing `await` is appropriate and improves performance. --- `91-96`: **LGTM! Correctly removed unnecessary await.** Consistent with the previous change, removing `await` from the synchronous `createQueryBuilder()` call is correct. --- `129-134`: **LGTM! Correctly removed unnecessary await.** Another correct removal of `await` from the synchronous `createQueryBuilder()` method call. </details> <details> <summary>test/unit/util/orm-utils.ts (2)</summary> `2-2`: **LGTM! Added import for improved type safety.** Adding the `DeepPartial` import supports the explicit type annotations added to the test. --- `125-131`: **LGTM! Enhanced type safety with explicit annotations.** The explicit `DeepPartial<typeof c>` type annotations improve type safety by clearly indicating that `a` and `b` are partial versions of the complete object structure `c`. This aligns with the PR's goal of stricter type-checking. </details> <details> <summary>test/functional/query-builder/not/query-builder-not.ts (1)</summary> `27-27`: **LGTM: Correctly removed unnecessary `await` keywords** The `getSql()` method returns a synchronous string, so removing the `await` keywords is the correct fix. This change aligns with the stricter type-checking objectives of the PR. Also applies to: 53-53 </details> <details> <summary>test/functional/query-builder/brackets/query-builder-brackets.ts (1)</summary> `24-67`: **LGTM: Appropriate refactoring from async to sync pattern** The refactoring from `Promise.all` to a synchronous `for` loop is correct since this test only calls `getSql()`, which is synchronous. This simplifies the code and removes unnecessary async complexity while maintaining the same test logic. </details> <details> <summary>test/utils/xfail.ts (1)</summary> `12-12`: **LGTM: Improved type accuracy** Changing the return type from `Func` to `AsyncFunc` is correct since the function returns `PromiseLike<any>` (line 13). This improves type safety and aligns with the stricter type-checking objectives. </details> <details> <summary>src/query-builder/RelationLoader.ts (1)</summary> `406-406`: **LGTM: Appropriate ESLint suppressions for lazy loading** The ESLint suppressions for `no-floating-promises` are justified in this context. These are intentional fire-and-forget promises used in lazy loading scenarios where the promises populate entity data asynchronously in the background and don't require explicit awaiting or error handling at the call site. Also applies to: 445-445 </details> <details> <summary>src/cli.ts (1)</summary> `19-19`: **LGTM: Appropriate CLI argument parsing improvements** The changes are well-suited for a CLI context: - Using `.parse()` instead of `.argv` is more explicit about triggering argument parsing and command execution - The ESLint suppression is justified since the top-level yargs promise doesn't need to be awaited in a CLI script - the process will naturally exit when the command completes Also applies to: 42-42 </details> <details> <summary>test/functional/columns/value-transformer/value-transformer.ts (4)</summary> `80-80`: **Good conversion to expect assertion style.** The change from `should` to `expect` assertion style improves consistency across the test suite. --- `97-99`: **Good conversion to expect assertion style.** The multi-line expect assertion is properly formatted and consistent with the new assertion style. --- `114-114`: **Good conversion to expect assertion style.** The assertion style conversion is consistent and properly implemented. --- `72-72`: **Verify that getRepository() is synchronous.** The removal of `await` assumes that `connection.getRepository()` is a synchronous method. Please confirm this is correct. ```shell #!/bin/bash # Description: Verify that getRepository method is synchronous by checking its return type ast-grep --pattern 'getRepository($_): $RETURN_TYPE { $$$ }'test/github-issues/11440/issue-11440.ts (3)
24-24: Good refactoring to arrow function.The conversion to an async arrow function that directly returns the Promise.all call simplifies the code structure.
108-108: Proper closing syntax for arrow function.The closing syntax matches the arrow function structure introduced on line 24.
85-85: Verify SQL syntax correctness.The removal of parentheses around the WHERE clause condition changes the SQL from
WHERE ("Post"."title" IS DISTINCT FROM EXCLUDED."title")toWHERE "Post"."title" IS DISTINCT FROM EXCLUDED."title". Please confirm this is valid SQL syntax for the target databases.Is the SQL syntax `WHERE "Post"."title" IS DISTINCT FROM EXCLUDED."title"` valid for PostgreSQL and CockroachDB without parentheses?test/github-issues/4185/subscriber/ExtendedAfterLoadSubscriber.ts (1)
13-15: Verify that afterLoad can be synchronous.The removal of
asyncfrom theafterLoadmethod seems appropriate since it only performs synchronous property assignment. However, please confirm that theEntitySubscriberInterfaceallows synchronousafterLoadmethods.#!/bin/bash # Description: Check the EntitySubscriberInterface definition for afterLoad method signature ast-grep --pattern 'interface EntitySubscriberInterface<Entity> { $$$ afterLoad?($_, $_): $RETURN_TYPE $$$ }'test/github-issues/4980/issue-4980.ts (4)
23-32: Good refactoring from parallel to sequential execution.The conversion from
Promise.allwithmapto a synchronousforEachloop simplifies the code while preserving all assertion logic. The sequential execution pattern is appropriate since the operations on different connections are independent.
34-45: Consistent refactoring pattern.The same beneficial conversion pattern applied to the second test case, maintaining all existing assertions while simplifying control flow.
47-67: Good preservation of Oracle-specific logic.The refactoring correctly preserves the Oracle-specific conditional checks for
onUpdatebehavior while converting to the simpler synchronous loop pattern.
69-90: Complete and consistent refactoring.The final test case follows the same pattern, maintaining all logic including Oracle-specific conditions while benefiting from the simplified synchronous execution.
test/github-issues/7203/issue-7203.ts (1)
21-30: I’ll search the codebase for wherecreateQueryBuilderis defined to confirm its return type.#!/bin/bash # Search for the definition of createQueryBuilder to see if it's synchronous rg -n "createQueryBuilder(" -C3 --type tstest/functional/query-builder/subquery/query-builder-subquery.ts (1)
79-79: Excellent cleanup of unnecessary await keywords.Correctly removes
awaitfrom synchronous query builder creation operations. These methods return immediately and don't require async handling, improving code clarity and performance.Also applies to: 164-164, 190-190, 220-220, 257-257
test/github-issues/4842/issue-4842.ts (1)
22-32: Good refactoring to remove unnecessary async complexity.The conversion from
Promise.allwith async callbacks to a synchronousforEachis appropriate since all the operations (createQueryBuilder,distinct,disableEscaping,getSql,clone) are synchronous. This simplifies the test without changing its logic or assertions.test/github-issues/521/issue-521.ts (1)
21-36: Appropriate simplification of test logic.The refactoring from async
Promise.allto synchronousforEachis correct since all query builder operations (createQueryBuilder,update,where,getQueryAndParameters) are synchronous. This improves test readability while maintaining the same assertions.src/connection/options-reader/ConnectionOptionsEnvReader.ts (1)
20-20: Verify all callers have been updated for the API change.The method signature change from
async read(): Promise<DataSourceOptions[]>toread(): DataSourceOptions[]is appropriate since the method body was always synchronous - it only reads environment variables and constructs configuration objects. However, this is a breaking API change.Please verify that all callers of
ConnectionOptionsEnvReader.read()have been updated to removeawait:#!/bin/bash # Search for calls to ConnectionOptionsEnvReader read method rg -A 3 -B 3 "ConnectionOptionsEnvReader.*\.read\(\)" # Also search for any remaining awaits on read() rg -A 2 -B 2 "await.*\.read\(\)" --type tstest/github-issues/5067/issue-5067.ts (1)
19-31: Let's broaden our search forcreateParameterdefinitions and async variants:#!/bin/bash # Find all occurrences of createParameter across the repo rg "createParameter" -n # Specifically look for async definitions rg "async[[:space:]]+createParameter" -ntest/other-issues/mongodb-entity-change-in-listeners/entity/Post.ts (2)
22-24: Correct removal of unnecessary async keyword.The
beforeUpdate()method only performs a synchronous string concatenation (this.title += "!"), so removing theasynckeyword is appropriate and aligns with TypeORM's lifecycle hook patterns.
29-31: Good simplification of lifecycle hook.The
afterLoad()method only sets a boolean property synchronousl (this.loaded = true), so removing theasynckeyword is correct. The@AfterLoaddecorator expects synchronous methods for simple property updates like this.test/other-issues/preventing-injection/preventing-injection.ts (2)
114-128: LGTM! Appropriate refactoring from async to sync.The QueryBuilder methods
.take()and.skip()throw errors synchronously when given invalid input, making the change fromPromise.allwith async iteration to synchronousforEachappropriate and more readable.
130-144: LGTM! Consistent refactoring pattern.The QueryBuilder
.orderBy()method also throws errors synchronously for invalid input, making this refactoring consistent with the previous test case and improving code clarity.test/github-issues/3118/issue-3118.ts (1)
158-178: LGTM! Correct refactoring and improved assertion style.The metadata access operations (
connection.driver.maxAliasLengthandconnection.entityMetadatas) are synchronous, making the refactoring appropriate. The assertion change from.to.be.trueto.to.equal(true)is also an improvement for clarity.test/github-issues/3443/issue-3443.ts (1)
23-42: LGTM! Appropriate conversion to synchronous iteration.The
dataSource.getMetadata()method and metadata property access are synchronous operations, making the refactoring from asyncPromise.allto synchronousforEachcorrect and more readable.test/github-issues/108/issue-108.ts (2)
17-17: LGTM! Appropriate test setup for explicit synchronization testing.Adding
driverSpecific: { synchronize: false }is correct since this test explicitly calls and verifies theconnection.synchronize()method. Disabling automatic synchronization ensures the test has full control over when synchronization occurs.
22-22: LGTM! Typo fix and improved test assertion.Fixed the typo "unqiue" → "unique" and improved the test by explicitly verifying that
connection.synchronize()completes successfully using.to.eventually.be.fulfilled. This makes the test more meaningful and explicit about what it's testing.Also applies to: 25-26
test/functional/tree-tables/closure-table/closure-table-mysql.ts (3)
25-54: LGTM! Appropriate refactoring for synchronous metadata operations.The test only performs synchronous metadata access operations (accessing
dataSource.entityMetadatasand column properties), making the conversion from asyncPromise.allto synchronousforEachcorrect and more readable.
56-94: LGTM! Consistent refactoring pattern.Like the previous test, this only performs synchronous metadata access operations for testing column properties (zerofill, width, precision, scale), making the async-to-sync refactoring appropriate.
96-131: LGTM! Final consistent refactoring in the pattern.This test case follows the same pattern as the others, performing only synchronous metadata access operations for testing column properties (length, charset, collation). The refactoring is consistent and appropriate.
test/github-issues/1576/issue-1576.ts (1)
24-37: LGTM: Simplified test control flowThe refactoring from
Promise.allwith async callbacks to a synchronousforEachloop is appropriate here since the test logic only involves entity creation and property assertions without any actual asynchronous operations.test/github-issues/3857/issue-3857.ts (2)
1-11: LGTM: Improved import organizationThe import reordering groups related utilities together and moves frequently used imports like
expectandDataSourceto more prominent positions, improving code readability.
27-36: LGTM: Appropriate synchronous iterationConverting from
Promise.allwith async callbacks to synchronousmapis correct here since the test only accesses metadata properties and performs assertions without any asynchronous operations.test/github-issues/6977/issue-6977.ts (1)
23-35: LGTM: Excellent test simplificationThe refactoring achieves two improvements:
- Simplified control flow: Replacing
Promise.allwithforEachis appropriate since no async operations are performed- Cleaner assertions: Consolidating multiple individual assertions into a single
deep.equalcheck on a sorted array is more maintainable and provides clearer failure messagessrc/persistence/SubjectExecutor.ts (2)
657-667: LGTM: Improved async handling patternThe refactoring from manual Promise construction to an async function is cleaner and more maintainable. The sequential await pattern for nested set subjects is preserved while improving readability and following modern async/await best practices.
1115-1115: LGTM: Improved type safetyRemoving the type assertion indicates better type inference, likely from the stricter TypeScript configuration mentioned in the PR objectives. This improves type safety without affecting functionality.
test/github-issues/2216/issue-2216.ts (2)
37-50: LGTM: Appropriate synchronous conversionConverting this test to use synchronous
forEachis correct since it only performs sinon assertions without any async operations. The test logic is preserved exactly.
95-108: LGTM: Consistent synchronous patternThis change mirrors the previous test case, appropriately using synchronous iteration for assertion-only logic. Good judgment in selectively applying async-to-sync conversion only where no actual async operations occur.
test/github-issues/7932/issue-7932.ts (1)
61-76: Good refactoring to synchronous pattern.The conversion from
Promise.alltoforEachis appropriate here since the test is only accessing synchronous metadata properties (repo.metadata.ownColumns) rather than performing asynchronous database operations. This simplifies the test structure while preserving the same verification logic.src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts (2)
592-594: Good explicit type annotation.Adding the explicit generic type
<{ [idHash: string]: any[] }>to the reducer improves type safety and code clarity by making the accumulator structure explicit rather than relying on type inference.
625-625: Let’s inspect the surrounding conditional in RawSqlResultsToEntityTransformer to see howcolumn.referencedColumnis guarded:#!/bin/bash # Show context around the referencedColumn check and value assignment rg -n "column\.referencedColumn" -A10 -B5 src/query-builder/transformer/RawSqlResultsToEntityTransformer.tstest/github-issues/3256/subscriber/PostSubscriber.ts (2)
15-17: Correctly removed unnecessary async keyword.The
beforeInsertmethod performs only synchronous operations (setting theinsertedproperty), so removing theasynckeyword is appropriate and improves performance by avoiding unnecessary promise wrapping.
19-23: Correctly removed unnecessary async keyword.The
beforeUpdatemethod performs only synchronous operations (conditionally setting theupdatedproperty), so removing theasynckeyword is appropriate and improves performance.test/github-issues/8408/delete-orphans.ts (2)
1-8: Import organization improved.The import reordering enhances code organization without functional changes.
33-78: Good test consolidation with parallel execution.The restructuring from nested
describeblocks to a singleitblock usingPromise.allis appropriate here because:
- The test performs actual database operations (async) unlike metadata-only tests
- Parallel execution across connections is efficient and maintains test isolation
- The consolidated structure is cleaner while preserving all original test logic and assertions
- All the key test scenarios are maintained: entity creation, orphan deletion verification, and foreign key retention checks
sample/sample16-indexes/app.ts (1)
18-46: Excellent refactoring to modern async/await pattern.This refactoring significantly improves the code quality by:
- Better readability: async/await is more intuitive than promise chains
- Improved error handling: Separate try-catch blocks for initialization and entity operations provide clearer error context
- Proper resource cleanup: The finally block ensures data source destruction even if errors occur
- Modern patterns: The main() function pattern is a best practice for top-level async code
The
void main()correctly indicates intentional fire-and-forget execution while maintaining the same functionality as the original promise-based approach.test/github-issues/8026/issue-8026.ts (1)
25-42: LGTM - Logic is correct, but verify test execution compatibility.The refactoring from
Promise.allwith async mapping to a synchronousfor...ofloop is appropriate since the query builder operations are synchronous. However, ensure that changing the test function fromasyncto sync doesn't break the test runner's expectations.Run the following script to verify the test still executes properly:
#!/bin/bash # Description: Verify the test executes without issues after async to sync conversion # Test: Run this specific test file to ensure it executes properly npm test -- --grep "github issues > #8026"sample/sample17-versioning/app.ts (1)
17-48: Excellent refactoring to modern async/await pattern!This refactoring significantly improves the code by:
- Replacing promise chains with cleaner async/await syntax
- Adding proper error handling with try/catch blocks
- Ensuring resource cleanup in the finally block
- Using
void main()to properly invoke the async functionThe implementation follows modern Node.js best practices for database connection management.
sample/sample1-simple-entity/app.ts (1)
18-45: Consistent and well-structured async/await refactoring.The refactoring follows the same excellent pattern as other samples in this PR:
- Clean async/await syntax replacing promise chains
- Granular error handling for connection and save operations
- Early return on connection failure to avoid unnecessary operations
- Explicit resource cleanup with
dataSource.destroy()The consistency across sample applications will help developers understand the recommended patterns.
test/github-issues/512/issue-512.ts (2)
1-3: Improved imports and assertion style.The import consolidation and addition of
expectfrom chai improves consistency and readability.
22-37: Good refactoring with assertion style improvement, but verify test compatibility.The changes improve the code by:
- Simplifying the iteration from
Promise.allto synchronousforEach- Using more explicit
expect().to.deep.contain()assertion style- Maintaining the same test logic for table name escaping
However, similar to the previous test file, ensure that changing from
asyncto sync doesn't affect test runner compatibility.Run the following script to verify test execution:
#!/bin/bash # Description: Verify the test executes properly after refactoring # Test: Run this specific test to ensure it works with the new structure npm test -- --grep "github issues > #512"src/util/ImportUtils.ts (3)
7-7: Good type simplification.Removing the named tuple element simplifies the return type while maintaining the same functionality.
12-12: Appropriate ESLint directive for dynamic import.The ESLint directive is necessary here since
Functionconstructor is intentionally used to preserve theimport()statement after transpilation, avoiding conversion torequire().
21-23: Good optimization - removing unnecessary async wrapper.Converting
tryToRequirefrom async to sync is a good optimization sincerequire()is inherently synchronous. This removes unnecessary Promise overhead.Run the following script to verify this change doesn't break existing usage:
#!/bin/bash # Description: Check if any code depends on tryToRequire being async # Search for usage patterns that might expect async behavior rg -A 5 -B 5 "tryToRequire|importOrRequireFile" --type tseslint.config.mjs (1)
26-29: Good upgrade to stricter type checking configuration.The switch from
tseslint.configs.recommendedtotseslint.configs.recommendedTypeCheckedenables more comprehensive type checking. This aligns well with the PR's objective of adding stricter type-checking.test/functional/connection/connection.ts (3)
1-26: Excellent test modernization.The refactoring improves code consistency by:
- Switching to
expectassertions for better readability- Organizing imports more logically
- Using specific error types for better error handling
77-83: Improved async error handling.The use of
expect(...).to.eventually.be.rejectedWith()with specific error types likeCannotExecuteNotConnectedErrorprovides better error validation than generic error catching.
53-53: Consistent API usage.The change from
connection.isConnectedtoconnection.isInitializedreflects the updated DataSource API and maintains consistency with the newinitialize()/destroy()methods.test/github-issues/1369/subscriber/AbstractEntitySubscriber.ts (3)
17-19: Correct removal of unnecessary async.The
beforeInsertmethod doesn't perform any asynchronous operations, so removing theasynckeyword improves performance by avoiding unnecessary promise creation and aligns with the PR's goal of improving event loop handling.
21-25: Consistent async removal.Similar to
beforeInsert, thebeforeUpdatemethod doesn't need to be async since it only performs synchronous operations.
27-29: Improved encapsulation.Changing
updateFullNamefrom public to private is good encapsulation since it's only used internally within the subscriber class.test/github-issues/1716/issue-1716.ts (3)
119-128: Good test consolidation.Merging MySQL and MariaDB tests into a single describe block makes sense since they likely have similar date/time handling behavior. The
enabledDrivers: ["mysql", "mariadb"]ensures both drivers are tested when available.
45-116: Efficient concurrent test execution.Using
Promise.allwithconnections.mapensures that tests run concurrently across all available database connections, improving test performance while maintaining thorough coverage.
34-39: Explicit timezone configuration.Setting the timezone to UTC for PostgreSQL tests ensures consistent behavior across different environments and makes the test results more predictable.
src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts (1)
2065-2397: LGTM! Effective async-to-sync refactoring.The removal of
Promise.alland conversion to synchronousmapis appropriate here since the table creation logic doesn't perform any asynchronous operations. This change improves performance by eliminating unnecessary promise overhead while maintaining the same functionality.The refactoring aligns with the PR's objective of modernizing asynchronous handling patterns.
test/github-issues/8832/issue-8832.ts (4)
1-16: Clean import organization.The import reordering and cleanup improves code organization and readability.
20-31: Improved test lifecycle management.Converting from
afterEachtobefore/afterhooks for connection management is more appropriate since connections don't need to be recreated between individual tests. This reduces test execution time and improves clarity.Also applies to: 108-119, 162-173
63-71: Enhanced SQL query safety with tagged template literals.The conversion to tagged template literals (
connection.sql) improves SQL injection safety by properly parameterizing the database and table names. This is a significant security improvement over string concatenation.Also applies to: 135-143
55-57: Standard IPv6 address compression.The change from expanded (
2001:0db8:0000:0000:0000:ff00:0042:8329) to compressed (2001:db8::ff00:42:8329) IPv6 format follows RFC standards and is the preferred representation.src/driver/sap/SapQueryRunner.ts (1)
2623-2927: LGTM! Correct synchronous refactoringThe refactoring from
Promise.allto synchronous.map()is appropriate here since all operations within the map callback are synchronous. The asynchronous database queries are already resolved before the mapping operation, making the async wrapper unnecessary.src/util/OrmUtils.ts (4)
1-1: Good addition for type safetyThe
DeepPartialimport enhances type safety for themergeDeepmethod, allowing more precise typing of partial objects being merged.
51-56: Excellent type safety improvementChanging the criteria parameter type from implicit
anyto(item: T) => unknownis a good practice. Usingunknowninstead ofanyenforces type checking and prevents potential runtime errors.
84-87: Well-designed generic implementationThe generic type parameter and
DeepPartial<T>usage provide excellent type safety while maintaining flexibility. The method signature now correctly enforces that merged sources must be structurally compatible with the target type.
108-108: Necessary type assertion for proper inferenceThe
as Tcast is appropriate here sinceObject.createreturnsany. This ensures proper type inference for callers while maintaining the shallow copy semantics.
| it("should save entities properly", () => | ||
| Promise.all( | ||
| connections.map(async (connection) => { | ||
| const post = new Post() | ||
| const comment = new Comment() | ||
| const value = new Value() | ||
|
|
||
| value.description = "description" | ||
| comment.values = [value] | ||
| post.comments = [comment] | ||
| value.description = "description" | ||
| comment.values = [value] | ||
| post.comments = [comment] | ||
|
|
||
| await connection.mongoManager.save(post) | ||
| await connection.mongoManager.save(post) | ||
|
|
||
| const postRepo = await connection.getRepository(Post) | ||
| const posts = await postRepo.find({}) | ||
| posts.forEach((post) => expect(post).to.be.instanceof(Post)) | ||
| } | ||
| }) | ||
| const postRepo = connection.getRepository(Post) | ||
| const posts = await postRepo.find({}) | ||
| posts.forEach((post) => expect(post).to.be.instanceof(Post)) | ||
| }), | ||
| )) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent async execution pattern across the PR.
This test moves from sequential to parallel execution using Promise.all, which improves performance. However, other files in this PR (e.g., test/github-issues/587/issue-587.ts) are moving in the opposite direction from parallel to sequential execution.
Consider establishing a consistent pattern across all test files. For independent connection tests like these, parallel execution is generally preferable for performance.
🤖 Prompt for AI Agents
In test/github-issues/9049/issue-9049.ts around lines 25 to 42, the test uses
Promise.all to run connection tests in parallel, but other test files in the PR
use sequential execution. To maintain consistency and improve performance for
independent connection tests, keep the parallel execution pattern here and
update other test files to also use Promise.all for running connection tests
concurrently.
There was a problem hiding this comment.
@OSA413 @sgarner what do you think about this? I was also wondering while fixing require-await if I should do this change.
@coderabbitai's comment about performance is not valid in our case, because we are using Promise.all(dataSources.map(async (dataSource) => {...})) for async tests and dataSources.forEach((dataSource) => {...}) for non-async, I was rather thinking about code style and consistency.
It would be nice if we had it.each like in jest. There's a package called it-each which would add this, but hasn't received updates in about 2 years. We can also talk about this in our chat
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Would it help to have our own test helper function wrapping the Promises.all part, to make all such tests consistent?
Something like...
it("should save entities properly", withDataSources(dataSources, async dataSource => {
// async code here to run in parallel with each data source
}))There was a problem hiding this comment.
I was thinking we could add it-each. Then, the tests would look like this:
it.each(
dataSources,
'should save entities properly for driver %s',
['type'], // replaces '%s' in title with dataSource.type
(dataSource) => { // can also be async
...
})If we do something like this, it should definitely be a dedicated PR, since there are a lot of tests that need updating. Luckily, indentation shouldn't change for this example.
It's just a suggestion, I've only worked with jest.each until now.
While there are some things I don't like about the way jest runs the tests (which they apparently fixed last week), I actually like to write tests with it. There are some codemods that convert mocha+chai to jest. Also since vitest is compatible with the jest APIs, we could use vitest to run the tests. So there are some options, but I'd say let's chat when one of us has time to look into them :)
There was a problem hiding this comment.
I am personally more used to writing tests in jest, so would support a change in that direction :)
Otherwise though - I think rather than using an unmaintained third-party package for it-each, it would be better to create something ourselves that is tuned for our requirements, since this pattern of looping through data sources is repeated so many times across our tests.
alumni
force-pushed
the
stricter-typechecking
branch
from
e5304f8 to
12614ba
Compare
alumni
force-pushed
the
stricter-typechecking
branch
from
12614ba to
3b8f7cd
Compare
…orm#11540) * refactor: minor type improvements * chore: add type-checked eslint rules * fix: enable no-misused-promises * fix: enable no-floating-promises * fix: enable await-thenable * fix: enable require-await * fix: enable no-misused-new * refactor: enable no-namespace * refactor: enable tseslint eslint recommended * code review
4 participants
Description of change
Added rules from typescript-eslint's recommended type checked config.
Adjusted code to follow the new eslint rules.
Possibly fixes some issues, will try to add links if I discover any. Fixes #10370.
Pull-Request Checklist
masterbranchFixes #00000Summary by CodeRabbit
Refactor
Bug Fixes
Style
Chores
Documentation
Tests