Fix constructor to support all Sequelize options

[bilby91]

Fixes #119

[codecov-io]

Codecov Report

Merging #120 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   96.48%   96.49%   +<.01%     
==========================================
  Files          99       99              
  Lines         911      912       +1     
  Branches      121      122       +1     
==========================================
+ Hits          879      880       +1     
  Misses         11       11              
  Partials       21       21

Impacted Files	Coverage Δ
lib/models/v3/Sequelize.ts	98.18% <100%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27efc23...1de8ae4. Read the comment docs.

[bilby91]

@RobinBuschmann Hey, what do you think ? I think this is the last issue that is blocking us to get this in production! A beta.2 today would be AWESOME!

[RobinBuschmann]

@bilby91 Whats the purpose of adding the dialectoption here?

[bilby91]

I'm not sure why it was working before. v4 requires a dialect to be explicitly defined.

[RobinBuschmann]

Strange, there seem to have happened an issue earlier caused by another pull request I think.

There is a ISequelizeValidationOnlyConfig which is missing in:

export type SequelizeConfig = ISequelizeConfig | ISequelizeUriConfig | ISequelizeDbNameConfig;

When you add ISequelizeValidationOnlyConfig to SequelizeConfig it should work without dialect: 'sqlite'.

[bilby91]

@RobinBuschmann Want me to add it ?

[RobinBuschmann]

It depends... does it build without dialect: 'sqlite',? Cause this line does not fit the actual api, because it is validationOnly without the need to specify database configuration.

If it is building, then we should create another pull request for this issue.

[bilby91]

@RobinBuschmann Changing this export type SequelizeConfig = ISequelizeConfig | ISequelizeUriConfig | ISequelizeDbNameConfig | ISequelizeValidationOnlyConfig; didn't fix the issue. I think the problem is different. The thing is that now the url constructor passes the options always and in v4 you need to always define the dialect. Make sense ?

[bilby91]

If we remove the dialect: 'sqlite' tests won't pass. And I think it makes sense that they don't pass without that option.

[RobinBuschmann]

When validationOnly: true is defined, these options (e.g. dialect) will be added automatically: https://github.com/RobinBuschmann/sequelize-typescript/blob/master/lib/models/BaseSequelize.ts#L61

[RobinBuschmann]

validationOnly: true makes it possible to use sequelize-typescript without connecting to any database. Thats why it should not be necessary to define database options.

[RobinBuschmann]

Here the reason why it compiles with validationOnly: true and without dialect: https://github.com/RobinBuschmann/sequelize-typescript/blob/master/lib/models/Sequelize.d.ts#L11

[bilby91]

@RobinBuschmann Good. Got it. It was not working localy because I though npm run test was compiling the code :(.

Now it should be good to go

[bilby91]

Thanks for the awesome feedback loop @RobinBuschmann . Can we get a beta.2 🎉 ?

[RobinBuschmann]

Thank you for fixing this :)

Its done sequelize-typescript@0.5.0-beta.2 🎉

[bilby91]

@RobinBuschmann Thanks!!