Skip to content
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

Fix some issues with Sequelize compatibility #39

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

robpre
Copy link
Contributor

@robpre robpre commented Nov 22, 2017

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

We need to return the object used to store the datatypes, otherwise sequelizeInstance.import is broken.
@robpre robpre changed the title Fix issue related to mimicking sequelize.import Fix some issues with Sequelize compatibility Nov 22, 2017
@robpre
Copy link
Contributor Author

robpre commented Nov 22, 2017

I've added another commit in this branch which adds the modelManager to the mocked Sequelize, I can separate them if that's preferred.

@LoveAndCoding
Copy link
Contributor

Hi. Thanks for your work on this.

Yeah, if you could rollback the modelManager change I can approve the import change on it's own. That commit looks good to go.

As for the modelManager change, I'm not sure I'll be taking that one yet. The modelManager is an undocumented feature in Sequelize at the moment, so it isn't too high on the list of things to mock out. Also, when we do, we will likely want to just recreate it. Even if it is independent in the Sequelize code base, I'd like to avoid having the mock library calling into any of the functionality of the real library itself. It can get messy, and potentially have unintended side effects that I'm trying to avoid.

Is modelManager something you use in your code somehow?

@LoveAndCoding
Copy link
Contributor

Also, I'll try and get an .editorconfig file in there so you don't have to worry about the whitespace issue.

Copy link
Contributor

@LoveAndCoding LoveAndCoding left a 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.

@robpre
Copy link
Contributor Author

robpre commented Nov 23, 2017

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 sequelize so we reduce the amount of upkeep required. It's useful to know what pattern this project is going for though, thanks. I'll copy across the file in my fork.

I make use of modelManager to retrieve the models from my sequelize instance, it's part of the public API, so I didn't really think about it being an internal feature. I find there's lots of things missing from the sequelize docs :)

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.

@robpre
Copy link
Contributor Author

robpre commented Nov 23, 2017

I'm also using latest stable node and npm 5.5.1, what version of node + npm was this repo targeting?

@LoveAndCoding
Copy link
Contributor

Is there a reason you use the modelManager instead of just using sequelize.model('Foo') or sequelize.models.Foo to access it? Both of those functions are in their documentation and are supported by our mocking code, so I'm just curious as to what different behavior you relying on.

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).

@LoveAndCoding LoveAndCoding merged commit c3e573b into BlinkUX:master Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants