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
feat(server): support advanced sequelize options #326
feat(server): support advanced sequelize options #326
Conversation
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.
thanks @cannjeff this sgtm! just a few minor changes
@@ -80,6 +80,7 @@ function createSequelize(options) { | |||
const sequelizeOptions = { | |||
operatorsAliases: false, | |||
logging: () => {}, | |||
...options.sequelizeOptions, |
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.
let's at least move operatorsAliases: false
down below this so it can't be overriden, we don't want folks to enable that and all of the work has already been done in LHCI to make due without them
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.
Good call. I wasn't sure which settings you specifically wouldn't want configurable. What about logging
does it make sense to leave that overridable?
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.
logging
is fine with me, it's just unnecessary noise but if folks want more insight into what it's doing that's fine with me :)
docs/configuration.md
Outdated
const fs = require('fs'); | ||
|
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.
const fs = require('fs'); |
this example doesn't use fs
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.
That's what I get for copy/paste... Good catch.
8cc2222
to
bd1fbe2
Compare
Addressed all comments. Thanks! |
thanks @cannjeff! 🎉 |
Adds the ability to provide advanced configuration options for Sequelize.