-
Notifications
You must be signed in to change notification settings - Fork 72
Fix some issues with Sequelize compatibility #39
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
Conversation
We need to return the object used to store the datatypes, otherwise sequelizeInstance.import is broken.
I've added another commit in this branch which adds the modelManager to the mocked Sequelize, I can separate them if that's preferred. |
a4b01b0
to
61af9dd
Compare
Hi. Thanks for your work on this. Yeah, if you could rollback the As for the Is |
Also, I'll try and get an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rollback the modelManager
change and this is good to go.
Sure, I'll take out that commit, I am also noticing a bunch of other inconsistencies with the real sequelize. I'll keep them in my fork and we can cherry-pick things across. To me I would prefer to use features from the original I make use of Some of my imports look like const Foo = modelManager.getModel('Foo');
const Bar = modelManager.getModel('Bar');
const Baz = modelManager.getModel('Baz');
const FooBarBazQux = modelManager.getModel('FooBarBazQux'); I've been thinking about refactoring it, but hadn't settled on a pattern I liked yet. |
I'm also using latest stable node and npm 5.5.1, what version of node + npm was this repo targeting? |
Is there a reason you use the I base mostly on mocking behavior from the documentation for Sequelize and using that as a base. I always try and check how things work under the hood so I can generally cover most of the same bases, but I really worry most about documented features and ignore undocumented ones, even if they are a part of public APIs. Exceptions happen always, especially if a Sequelize contributor recommends something in an issue, but I try to keep it to documented public API because I figure that is the stable API which is unlikely to change significantly between minor versions. As for Node/NPM version, I try to maintain the compatibility with the same level as Sequelize. Sequelize v3 doesn't list specific support but seems to avoid ES6 features in the code base. So far I've been avoiding ES6 for now to match their v3 code. Sequelize v4 is obviously a different beast and requires Node v4 and above. It uses ES6+ features heavily and I'm still trying to figure out if I can handle both appropriately in the mocking interface (see #33 for more info). |
We need to return the object used to store the datatypes, otherwise sequelizeInstance.import is broken.
See here for more info
Apologies for the whitespace diff, my editor removes them automatically. View the diff here without them