fix(objection): fix decorators to allow to work with circular imports in esbuild #2974
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Sequence DiagramsequenceDiagram
participant Developer
participant BelongsToOne
participant RelatesTo
participant CreateRelationshipMapping
Developer->>BelongsToOne: Define relationship
BelongsToOne->>RelatesTo: Pass model and options
RelatesTo->>CreateRelationshipMapping: Create relationship mapping
CreateRelationshipMapping-->>RelatesTo: Return mapping configuration
RelatesTo-->>BelongsToOne: Apply relationship
BelongsToOne-->>Developer: Configured relationship
This sequence diagram illustrates the high-level flow of how the enhanced ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/orm/objection/src/decorators/belongsToOne.spec.ts (1)
Line range hint
1-111: Consider enhancing test coverage for robustness.While the current tests effectively cover the happy path scenarios, consider adding the following test categories for more comprehensive coverage:
- Runtime Type Validation:
// Test invalid model types @BelongsToOne(() => String) // Should fail @BelongsToOne(() => {}) // Should fail
- Circular Dependency Resolution:
// Test circular dependencies @Entity("author") class Author { @BelongsToOne(() => Book) book?: Book; } @Entity("book") class Book { @BelongsToOne(() => Author) author?: Author; }
- Error Handling:
// Test decorator application on invalid property types @BelongsToOne(() => User) user: string; // Should fail, not a Model type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
packages/orm/objection/src/decorators/belongsToOne.spec.ts(1 hunks)packages/orm/objection/src/decorators/belongsToOne.ts(1 hunks)packages/orm/objection/src/decorators/relatesTo.ts(0 hunks)packages/orm/objection/src/domain/RelationshipOpts.ts(1 hunks)packages/orm/objection/src/utils/createRelationshipMapping.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/orm/objection/src/decorators/relatesTo.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/orm/objection/src/utils/createRelationshipMapping.ts
[error] 12-12: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/orm/objection/src/decorators/belongsToOne.ts
[error] 44-44: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 45-45: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (9)
packages/orm/objection/src/decorators/belongsToOne.ts (7)
1-1: No immediate concerns about importing from "node:os".
It doesn’t appear thattypeis used below. If this import is unnecessary, consider removing it.
3-5: Imports look good.
These imports reflect the new references needed by the revised decorator logic.
10-13: Documentation block check.
These doc comments for the@BelongsToOnedecorator param are concise and clear.
14-21: Function overload definitions forBelongsToOne.
Providing separate overloads for different parameter types helps ensure clarity. Everything looks good here.
22-29: Additional function overload forBelongsToOne.
This overload continues to maintain backward compatibility while adding extra flexibility.
30-37: Documentation block check.
The doc comments offer good clarity on the parameterstypeOrOptsandopts.
48-54: Final computation of options looks good.
MergingoptswithtypeOrOptsto handle multiple argument forms appears correct, and usingisModelClassSpecifieris a well-structured approach.packages/orm/objection/src/utils/createRelationshipMapping.ts (1)
4-4: New import forisModelClassFactoryandRelationshipOpts.
This aligns with the approach of resolvingopts.typedynamically.packages/orm/objection/src/domain/RelationshipOpts.ts (1)
15-18: Type guard utility is well-structured.
isModelClassSpecifierneatly handles both string-based and function-based specifiers.
LonguCodes
force-pushed
the
fix-objection-esbuild-esm
branch
from
1e35834 to
ad2bfde
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
packages/orm/objection/src/decorators/belongsToOne.spec.ts(1 hunks)packages/orm/objection/src/decorators/belongsToOne.ts(1 hunks)packages/orm/objection/src/decorators/relatesTo.ts(0 hunks)packages/orm/objection/src/domain/RelationshipOpts.ts(1 hunks)packages/orm/objection/src/utils/createRelationshipMapping.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/orm/objection/src/decorators/relatesTo.ts
🧰 Additional context used
📓 Learnings (2)
packages/orm/objection/src/decorators/belongsToOne.spec.ts (2)
Learnt from: LonguCodes
PR: tsedio/tsed#2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:60-84
Timestamp: 2025-01-29T23:20:16.323Z
Learning: Testing circular dependencies in TypeScript/JavaScript requires specific compiler settings and a live runtime environment, as it cannot be effectively tested through unit tests alone. This is particularly relevant for bundler-specific behaviors like esbuild's handling of circular imports.
Learnt from: LonguCodes
PR: tsedio/tsed#2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:86-111
Timestamp: 2025-01-29T23:19:32.209Z
Learning: The test suite for `@BelongsToOne` decorator in `belongsToOne.spec.ts` focuses on positive cases of relationship mapping configuration. Error cases are not applicable to this test suite as they are likely handled at TypeScript compilation time or in separate validation layers.
packages/orm/objection/src/decorators/belongsToOne.ts (1)
Learnt from: LonguCodes
PR: tsedio/tsed#2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:86-111
Timestamp: 2025-01-29T23:19:32.209Z
Learning: The test suite for `@BelongsToOne` decorator in `belongsToOne.spec.ts` focuses on positive cases of relationship mapping configuration. Error cases are not applicable to this test suite as they are likely handled at TypeScript compilation time or in separate validation layers.
🪛 Biome (1.9.4)
packages/orm/objection/src/utils/createRelationshipMapping.ts
[error] 12-12: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/orm/objection/src/decorators/belongsToOne.ts
[error] 42-42: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 43-43: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (6)
packages/orm/objection/src/decorators/belongsToOne.ts (2)
35-38: Overloads look goodThe multiple overloads ensure flexible usage, and the overall approach adequately covers different argument types.
42-43: Parameter reassignment confusingSimilar to a previous comment noted in past reviews, reassigning
typeOrOptsandoptscould reduce readability and risk unintended side effects. Consider using local variables for clarity.🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 43-43: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/orm/objection/src/utils/createRelationshipMapping.ts (1)
12-17: Avoid parameter reassignmentReassigning
optsmakes it harder to follow the flow of data. A best practice is to store the transformed value in a new variable rather than overriding the existing parameter.🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/orm/objection/src/domain/RelationshipOpts.ts (1)
16-18: Logic is correctThis utility function accurately checks whether the input is a valid model class specifier.
packages/orm/objection/src/decorators/belongsToOne.spec.ts (2)
60-84: LGTM! Test case effectively validates the circular import fix.The test case properly validates the new function-style model specification syntax
() => User, which directly addresses the PR's objective of fixing circular import issues in esbuild.
86-111: LGTM! Test case provides comprehensive coverage.The test case effectively validates the combined usage of function-style model specification with custom relationship paths, ensuring both features work together seamlessly.
| it("should set custom relationship model", () => { | ||
| @Entity("user") | ||
| class User extends Model { | ||
| @IdColumn() | ||
| id!: string; | ||
| } | ||
| @Entity("movie") | ||
| class Movie extends Model { | ||
| @IdColumn() | ||
| id!: string; | ||
| userId!: string; | ||
| @BelongsToOne(() => User) | ||
| user?: User; | ||
| } | ||
| expect(Movie.relationMappings).toEqual({ | ||
| user: { | ||
| relation: Model.BelongsToOneRelation, | ||
| modelClass: User, | ||
| join: { | ||
| from: "movie.userId", | ||
| to: "user.id" | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| it("should set custom relationship model and relationship path", () => { | ||
| @Entity("user") | ||
| class User extends Model { | ||
| @IdColumn() | ||
| id!: string; | ||
| userId!: string; | ||
| } | ||
| @Entity("movie") | ||
| class Movie extends Model { | ||
| @IdColumn() | ||
| id!: string; | ||
| ownerId!: string; | ||
| @BelongsToOne(() => User, {from: "ownerId", to: "userId"}) | ||
| user?: User; | ||
| } | ||
| expect(Movie.relationMappings).toEqual({ | ||
| user: { | ||
| relation: Model.BelongsToOneRelation, | ||
| modelClass: User, | ||
| join: { | ||
| from: "movie.ownerId", | ||
| to: "user.userId" | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing test descriptions for clarity.
The test descriptions could be more specific about the features being tested. Consider:
- "should set custom relationship model using function syntax for circular dependency support"
- "should combine function syntax with custom relationship path configuration"
LonguCodes
force-pushed
the
fix-objection-esbuild-esm
branch
from
ad2bfde to
fbc0ed8
Compare
|
@Romakita Would you be able to run the CI again? I've added coverage for the missing lines |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
packages/orm/objection/src/decorators/belongsToOne.spec.ts(1 hunks)packages/orm/objection/src/decorators/belongsToOne.ts(1 hunks)packages/orm/objection/src/decorators/relatesTo.ts(0 hunks)packages/orm/objection/src/domain/RelationshipOpts.ts(1 hunks)packages/orm/objection/src/utils/createRelationshipMapping.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/orm/objection/src/decorators/relatesTo.ts
🧰 Additional context used
📓 Learnings (2)
packages/orm/objection/src/decorators/belongsToOne.ts (1)
Learnt from: LonguCodes
PR: tsedio/tsed#2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:86-111
Timestamp: 2025-01-29T23:19:32.209Z
Learning: The test suite for `@BelongsToOne` decorator in `belongsToOne.spec.ts` focuses on positive cases of relationship mapping configuration. Error cases are not applicable to this test suite as they are likely handled at TypeScript compilation time or in separate validation layers.
packages/orm/objection/src/decorators/belongsToOne.spec.ts (2)
Learnt from: LonguCodes
PR: tsedio/tsed#2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:60-84
Timestamp: 2025-01-29T23:20:16.323Z
Learning: Testing circular dependencies in TypeScript/JavaScript requires specific compiler settings and a live runtime environment, as it cannot be effectively tested through unit tests alone. This is particularly relevant for bundler-specific behaviors like esbuild's handling of circular imports.
Learnt from: LonguCodes
PR: tsedio/tsed#2974
File: packages/orm/objection/src/decorators/belongsToOne.spec.ts:86-111
Timestamp: 2025-01-29T23:19:32.209Z
Learning: The test suite for `@BelongsToOne` decorator in `belongsToOne.spec.ts` focuses on positive cases of relationship mapping configuration. Error cases are not applicable to this test suite as they are likely handled at TypeScript compilation time or in separate validation layers.
🪛 Biome (1.9.4)
packages/orm/objection/src/utils/createRelationshipMapping.ts
[error] 12-12: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
packages/orm/objection/src/decorators/belongsToOne.ts
[error] 42-42: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 43-43: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
🔇 Additional comments (10)
packages/orm/objection/src/decorators/belongsToOne.ts (4)
1-3: Imports look good.
No issues noted with the updated imports ofModel,ModelClassSpecifier,isModelClassSpecifier,RelationshipOpts, andRelationshipOptsWithoutThrough.
12-27: Overload signatures are well-defined.
The multiple overloads forBelongsToOneadd clarity and flexibility without breaking existing usage. Nice work keeping them consistent.
35-44: Avoid reassigning function parameters.
ReassigningtypeOrOptsandopts(lines 42–43) is confusing and flagged by the lint rule.Use a local variable instead, for instance:
if (!typeOrOpts && opts) { - typeOrOpts = opts; - opts = undefined; + const newOpts = opts; + return RelatesTo(Model.BelongsToOneRelation, newOpts); }🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 43-43: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
46-50: Dynamic options assembly is clear.
The logic concisely mergestypeOrOptsintooptswhen it’s aModelClassSpecifier. The approach is clean and easy to maintain.packages/orm/objection/src/utils/createRelationshipMapping.ts (3)
4-4: Import statement is aligned with domain usage.
Kudos for maintaining consistency in your import references forRelationshipOpts.
11-17: Avoid reassigning theoptsparameter.
This practice is flagged by your linter (lint/style/noParameterAssign) and can degrade clarity.Use a local variable to refine or transform
opts, for example:- opts = opts - ? { - ...opts, - type: isModelClassFactory(opts.type) ? opts.type() : opts.type - } - : undefined; + const resolvedOpts = opts + ? { + ...opts, + type: isModelClassFactory(opts.type) ? opts.type() : opts.type + } + : undefined; return { ... modelClass: resolvedOpts?.type || ... };🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
19-28: Overall mapping structure is coherent.
Good job returning a clear configuration object for the relationship with minimal nesting.packages/orm/objection/src/domain/RelationshipOpts.ts (1)
16-18: Type predicate is well-crafted.
TheisModelClassSpecifierfunction cleanly checks for string or function types. This will improve type safety in downstream logic.packages/orm/objection/src/decorators/belongsToOne.spec.ts (2)
60-84: LGTM! Test case effectively validates function syntax for model specification.The test case properly validates the new
@BelongsToOne(() => User)syntax, which is crucial for supporting circular imports in esbuild. The assertions comprehensively verify both the relationship type and mapping configuration.
112-137: LGTM! Test case effectively validates backward compatibility.The test case properly validates that the decorator maintains backward compatibility by supporting undefined as the first argument while still correctly processing the options in the second argument.
| /** | ||
| * | ||
| * @param type | ||
| * @decorator | ||
| * @objection | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider including usage examples in the JSDoc.
While these lines provide a brief overview, adding a small code snippet demonstrating how to use @BelongsToOne would aid consumers of this decorator.
| it("should set custom relationship model and relationship path", () => { | ||
| @Entity("user") | ||
| class User extends Model { | ||
| @IdColumn() | ||
| id!: string; | ||
| userId!: string; | ||
| } | ||
| @Entity("movie") | ||
| class Movie extends Model { | ||
| @IdColumn() | ||
| id!: string; | ||
| ownerId!: string; | ||
| @BelongsToOne(() => User, {from: "ownerId", to: "userId"}) | ||
| user?: User; | ||
| } | ||
| expect(Movie.relationMappings).toEqual({ | ||
| user: { | ||
| relation: Model.BelongsToOneRelation, | ||
| modelClass: User, | ||
| join: { | ||
| from: "movie.ownerId", | ||
| to: "user.userId" | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing the test description for clarity.
While the test effectively validates the combined usage of function syntax and custom path configuration, consider making the test description more explicit about the features being tested:
it("should support circular imports by combining function syntax with custom relationship path", () => {
|
🎉 This PR is included in version 8.4.6 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Information
Fixes issue, where esbuild does not put decorators at the end, which results in usage of types (module references) before their initialization.
Todos
Summary by CodeRabbit
Tests
@BelongsToOnedecorator to validate:Improvements
BelongsToOnedecorator to accept various parameter typesRelatesTofunction for direct use ofopts.type