-
-
Notifications
You must be signed in to change notification settings - Fork 529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
moving model.associate outside classMethods due to sequelize@4.0.0 #470
Conversation
This desperately needs to be merged in :/ |
This will be a breaking change, so can't release to v3 |
I figured out the modification that needs to happen - all good in the interim :) |
I will post sample from v4 upgrade guide for others Previous : const Model = sequelize.define('Model', {
...
}, {
classMethods: {
associate: function (model) {...}
},
instanceMethods: {
someMethod: function () { ...}
}
}); New : const Model = sequelize.define('Model', {
...
});
// Class Method
Model.associate = function (models) {
...associate the models
};
// Instance Method
Model.prototype.someMethod = function () {..} |
@sushantdhiman thank you for a direct clarifying response and clear progress forward! |
I'm all for this change. I just spent an evening trying to figure out why my models and relationships were not working. I eventually compared the generated code from the cli to the example site to see the difference in the models. |
@sushantdhiman What would it take to get this merged? If @geluso doesn't want to fix the merge conflict could I branch off of his and fix it and create a new PR? Or if @geluso does fix the merge conflict is that all it would take to merge? Was wanting to see if I could help a bit move cli v4 along. |
@buddylindsey Please open a new branch and I will merge it, I am unable to find some time to complete #441 so let's fix incompatibilities first then think about other features |
Implement upgrade command
I'm trying to figure out if sequelize-cli is supposed to match breaking changes introduced in sequelize@4.0.0. If yes, the model.associate method needs to be moved outside
classMethods
becauseclassMethods
was removed in sequelize@4.0.0.Review my comment here: #468