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

feat(server): support advanced sequelize options #326

Merged

Conversation

cannjeff
Copy link
Contributor

Adds the ability to provide advanced configuration options for Sequelize.

Copy link
Collaborator

@patrickhulce patrickhulce left a 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,
Copy link
Collaborator

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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 Show resolved Hide resolved
const fs = require('fs');

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const fs = require('fs');

this example doesn't use fs

Copy link
Contributor Author

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.

@cannjeff
Copy link
Contributor Author

cannjeff commented May 26, 2020

Addressed all comments. Thanks!

@patrickhulce patrickhulce merged commit 18ab74e into GoogleChrome:master May 27, 2020
@patrickhulce
Copy link
Collaborator

thanks @cannjeff! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants