Pass models object in association annotations

[mschipperheyn]

I've had a number of issues with cyclical dependencies. TDLR, if I install the webpack CircularDependencyPlugin on my project (NextJS, apollo graphql, sequelize, react, typescript), the build would fail because of circular dependency errors caused by sequelize-typescript requiring circular model import. After trying various things, I had a hunch that passing an object with all initialized models object in @HasMany, @BelongsTo etc might resolve this issue (even though you still need the cyclical imports because of the direct reference to the associated model the model definition needs).

To my surprise, it worked. With the adjustments I made

@BelongsTo(() => User, {
    foreignKey: 'userId',
    as: 'user',
})
public user!: User;

becomes

@BelongsTo((models) => models.User, {
    foreignKey: 'userId',
    as: 'user',
})
public user!: User;

and the circular dependency errors disappear.

If this change makes sense, I can add some documentation. But for now, I'm assuming it might be rejected, so I'll leave it for consideration. Ideally, IMHO initialization and association should be separate passes so any reference to another model can occur through a models object and not require circular imports of models.

[geoffoliver]

@RobinBuschmann Please, please, please merge this PR. It would be a very welcome fix as this problem is actively breaking a NextJS project I'm trying to work on.

[brentkirkland]

This would be extremely helpful if merged

[Maximvdw]

This is a must have PR

[mschipperheyn]

@RobinBuschmann I would be perfectly willing to make the adjustments necessary to bring this in line with the main, but so far, only other other contributors have indicated interest. I'm assuming this major change in the API makes little sense with the new sequelize v7 coming up. I advised @ephys a while ago and I believe the issue this addresses will be considered for v7 sequelize.

[ephys]

Well since this repo is now under Sequelize I believe we could review and release it in v6

Since we haven't migrated this decorated to the main repo yet, this change will be part of v7 too

[mschipperheyn]

@ephys A good way to see if any problems occurs with recursive imports is build a simple project, e.g. nextjs, with webpack and use the circular-dependency-plugin. It will report any circular dependencies, which if reported, will tend to create problems. I will set up a test project for you to use.

[ephys]

Implemented in https://github.com/sequelize/sequelize/pull/15483 for v7, but with a slightly different approach: The ref forwarding function receives the current sequelize instance instead of a map of models

Feedback welcome :)

[mschipperheyn]

Implemented in sequelize/sequelize#15483 for v7, but with a slightly different approach: The ref forwarding function receives the current sequelize instance instead of a map of models

Feedback welcome :)

Seems like it would be equally useful. Maybe there are some additional advantages in having the sequelize instance available.

I guess I would add some order changes in the test to simulate the non predictable order of file imports for large model trees.

sequelize.addModels([Profile, User]);

[jose-verissimo]

:+1: Would love this merged sometime soon, thank you @mschipperheyn for the work